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

[Ferruh Yigit]

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 at nxp.com>
> Signed-off-by: Apeksha Gupta <apeksha.gupta at 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.

<...>

> +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.


> +	}
> +
> +	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.

> +
> +	/* 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.