Re: [dpdk-dev] [EXT] Re: [PATCH v3 2/5] net/enetfec: add UIO support

[Apeksha Gupta <apeksha.gupta@nxp.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, September 23, 2021 3:43 PM
> To: Apeksha Gupta <apeksha.gupta@nxp.com>;
> david.marchand@redhat.com; andrew.rybchenko@oktetlabs.ru;
> ferruh.yigit@intel.com
> Cc: dev@dpdk.org; Sachin Saxena <sachin.saxena@nxp.com>; Hemant
> Agrawal <hemant.agrawal@nxp.com>
> Subject: [EXT] Re: [dpdk-dev] [PATCH v3 2/5] net/enetfec: add UIO support
> 
> Caution: EXT Email
> 
> On 9/9/2021 9:43 PM, Apeksha Gupta wrote:
> > Implemented the fec-uio driver in kernel. enetfec PMD uses
> > UIO interface to interact with "fec-uio" driver implemented in
> > kernel for PHY initialisation and for mapping the allocated memory
> > of register & BD from kernel to DPDK which gives access to
> > non-cacheable memory for BD.
> >
> > Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> 
> <...>
> 
> > +static struct uio_job enetfec_uio_job;
> > +int count;
> > +
> 
> This is a global non-static variable with a very generic name, when DPDK
> application compiled all other dpdk libraries/drivers and applicaiton will have
> access to this variable.
> 
> Please make the variable 'static' if possible and add a driver prefix.
[Apeksha] Okay.

> 
> <...>
> 
> > +config_enetfec_uio(struct enetfec_private *fep)
> > +{
> > +     char uio_device_file_name[32];
> > +     struct uio_job *uio_job = NULL;
> > +
> > +     /* Mapping is done only one time */
> > +     if (count > 0) {
> > +             printf("Mapped!\n");
> > +             return 0;
> 
> 'printf' ? Should use logging macros.
[Apeksha] Yes, I agree.

> 
> 
> > +     }
> > +
> > +     uio_job = &enetfec_uio_job;
> > +
> > +     /* Find UIO device created by ENETFEC-UIO kernel driver */
> > +     memset(uio_device_file_name, 0, sizeof(uio_device_file_name));
> > +     snprintf(uio_device_file_name, sizeof(uio_device_file_name), "%s%d",
> > +                     FEC_UIO_DEVICE_FILE_NAME, uio_job->uio_minor_number);
> 
> I guess this assumes 'uio_minor_number' is '0', but how can you know? What
> if
> user bind another device first? Implemention your own uio handling is error
> prone.
[Apeksha] I do agree, We will update in v4 patch series.

> 
> > +
> > +     /* Open device file */
> > +     uio_job->uio_fd = open(uio_device_file_name, O_RDWR);
> > +     if (uio_job->uio_fd < 0) {
> > +             printf("US_UIO: Open Failed\n");
> > +             exit(1);
> 
> 'exit' ? Exit from all dpdk application because one of the drivers underneath
> failed? Please return an error.
[Apeksha] Okay.