> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Thursday, September 23, 2021 3:43 PM
> To: Apeksha Gupta <apeksha.gu...@nxp.com>;
> david.march...@redhat.com; andrew.rybche...@oktetlabs.ru;
> ferruh.yi...@intel.com
> Cc: dev@dpdk.org; Sachin Saxena <sachin.sax...@nxp.com>; Hemant
> Agrawal <hemant.agra...@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.sax...@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gu...@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.
