Date:   Wed, 10 Nov 2021 23:06:19 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Apeksha Gupta <apeksha.gupta@nxp.com>
Cc:     qiangqing.zhang@nxp.com, davem@davemloft.net, kuba@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-devel@linux.nxdi.nxp.com, LnxRevLi@nxp.com,
        sachin.saxena@nxp.com, hemant.agrawal@nxp.com, nipun.gupta@nxp.com
Subject: Re: [PATCH 2/5] net: fec: fec-uio driver
Message-ID: <YYxCWyLExiWgXf/L@lunn.ch>
References: <20211110054838.27907-1-apeksha.gupta@nxp.com>
 <20211110054838.27907-3-apeksha.gupta@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211110054838.27907-3-apeksha.gupta@nxp.com>
Precedence: bulk

On Wed, Nov 10, 2021 at 11:18:35AM +0530, Apeksha Gupta wrote:
> i.mx: fec-uio driver
> 
> This patch adds the userspace support. In this basic
> hardware initialization is performed in kernel via userspace
> input/output, while the majority of code is written in the
> userspace.

Where do i find this usespace code. Please include a URL to a git
repo.

> +static unsigned char macaddr[ETH_ALEN];
> +module_param_array(macaddr, byte, NULL, 0);
> +MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");

No module parameters please. Use the standard device tree bindings.

> +static int fec_enet_uio_init(struct net_device *ndev)
> +{
> +	unsigned int total_tx_ring_size = 0, total_rx_ring_size = 0;
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	unsigned int dsize = sizeof(struct bufdesc);
> +	unsigned short tx_ring_size, rx_ring_size;
> +	int ret, i;
> +
> +	/* Check mask of the streaming and coherent API */
> +	ret = dma_set_mask_and_coherent(&fep->pdev->dev, DMA_BIT_MASK(32));
> +	if (ret < 0) {
> +		dev_warn(&fep->pdev->dev, "No suitable DMA available\n");
> +		return ret;
> +	}
> +
> +	tx_ring_size = TX_RING_SIZE;
> +	rx_ring_size = RX_RING_SIZE;
> +
> +	for (i = 0; i <	FEC_ENET_MAX_TX_QS; i++)
> +		total_tx_ring_size += tx_ring_size;
> +	for (i = 0; i <	FEC_ENET_MAX_RX_QS; i++)
> +		total_rx_ring_size += rx_ring_size;
> +
> +	bd_size = (total_tx_ring_size + total_rx_ring_size) * dsize;

These are the buffer descriptors, not buffers themselves. I assume the
user space driver is allocating the buffer? And your userspace then
set the descriptor to point to user allocated memory? Or some other
memory in the address space, and overwrite whatever you want on the
next DMA? Or DMAing kernel memory out as frames?

> +static int
> +fec_enet_uio_probe(struct platform_device *pdev)
> +{
> +	struct fec_uio_devinfo *dev_info;
> +	const struct of_device_id *of_id;
> +	struct fec_enet_private *fep;
> +	struct net_device *ndev;
> +	u32 ecntl = ETHER_EN;
> +	static int dev_id;
> +	bool reset_again;
> +	int ret = 0;
> +
> +	/* Init network device */
> +	ndev = alloc_etherdev_mq(sizeof(struct fec_enet_private) +
> +				FEC_PRIV_SIZE, FEC_MAX_Q);

Why do you need this. This is not a netdev driver, since it does not
connect to the network stack.

> +static int
> +fec_enet_uio_remove(struct platform_device *pdev)
> +{
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +
> +	kfree(fec_dev);
> +	iounmap(fep->hwp);
> +	dma_free_coherent(&fep->pdev->dev, bd_size, cbd_base, bd_dma);

Don't you have to assume that the userspace driver has crashed and
burned, leaving the hardware in an undefined state. It could still be
receiving, into buffers we have no idea about. Don't you need to stop
the hardware, and then wait for all DMA activity to stop, and only
then can you free the buffer descriptors?

     Andrew