Re: [PATCH v2] usb: max-3421: Use driver data instead of maintaining a list of bound devices - David Mosberger-Tang

From: David Mosberger-Tang <davidm@egauge.net>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org, kernel@pengutronix.de
Subject: Re: [PATCH v2] usb: max-3421: Use driver data instead of maintaining a list of bound devices
Date: Tue, 19 Oct 2021 08:51:52 -0600	[thread overview]
Message-ID: <d3712663b47dbc45cb678d2abd015b2ecd2fef7a.camel@egauge.net> (raw)
In-Reply-To: <20211018204028.2914597-1-u.kleine-koenig@pengutronix.de>

Looks fine to me.  I could only compile-test though since I don't have
this hardware any more.

    Ack-by: David Mosberger-Tang <davidm@egauge.net>

Thanks,

  --david

On Mon, 2021-10-18 at 22:40 +0200, Uwe Kleine-König wrote:
> Instead of maintaining a single-linked list of devices that must be
> searched linearly in .remove() just use spi_set_drvdata() to remember the
> link between the spi device and the driver struct. Then the global list
> and the next member can be dropped.
> 
> This simplifies the driver, reduces the memory footprint and the time to
> search the list. Also it makes obvious that there is always a corresponding
> driver struct for a given device in .remove(), so the error path for
> !max3421_hcd can be dropped, too.
> 
> As a side effect this fixes a data inconsistency when .probe() races with
> itself for a second max3421 device in manipulating max3421_hcd_list. A
> similar race is fixed in .remove(), too.
> 
> Fixes: 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since (implicit) v1:
> 
>  - don't drop "max3421_hcd = hcd_to_max3421(hcd);", noticed by the
>    kernel test robot. Greg helped interpreting the kernel test robot's
>    finding.
> 
> As before, this patch is only build tested.
> 
> Best regards
> Uwe
>  drivers/usb/host/max3421-hcd.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
> index 59cc1bc7f12f..30de85a707fe 100644
> --- a/drivers/usb/host/max3421-hcd.c
> +++ b/drivers/usb/host/max3421-hcd.c
> @@ -125,8 +125,6 @@ struct max3421_hcd {
>  
>  	struct task_struct *spi_thread;
>  
> -	struct max3421_hcd *next;
> -
>  	enum max3421_rh_state rh_state;
>  	/* lower 16 bits contain port status, upper 16 bits the change mask: */
>  	u32 port_status;
> @@ -174,8 +172,6 @@ struct max3421_ep {
>  	u8 retransmit;			/* packet needs retransmission */
>  };
>  
> -static struct max3421_hcd *max3421_hcd_list;
> -
>  #define MAX3421_FIFO_SIZE	64
>  
>  #define MAX3421_SPI_DIR_RD	0	/* read register from MAX3421 */
> @@ -1882,9 +1878,8 @@ max3421_probe(struct spi_device *spi)
>  	}
>  	set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
>  	max3421_hcd = hcd_to_max3421(hcd);
> -	max3421_hcd->next = max3421_hcd_list;
> -	max3421_hcd_list = max3421_hcd;
>  	INIT_LIST_HEAD(&max3421_hcd->ep_list);
> +	spi_set_drvdata(spi, max3421_hcd);
>  
>  	max3421_hcd->tx = kmalloc(sizeof(*max3421_hcd->tx), GFP_KERNEL);
>  	if (!max3421_hcd->tx)
> @@ -1934,28 +1929,18 @@ max3421_probe(struct spi_device *spi)
>  static int
>  max3421_remove(struct spi_device *spi)
>  {
> -	struct max3421_hcd *max3421_hcd = NULL, **prev;
> -	struct usb_hcd *hcd = NULL;
> +	struct max3421_hcd *max3421_hcd;
> +	struct usb_hcd *hcd;
>  	unsigned long flags;
>  
> -	for (prev = &max3421_hcd_list; *prev; prev = &(*prev)->next) {
> -		max3421_hcd = *prev;
> -		hcd = max3421_to_hcd(max3421_hcd);
> -		if (hcd->self.controller == &spi->dev)
> -			break;
> -	}
> -	if (!max3421_hcd) {
> -		dev_err(&spi->dev, "no MAX3421 HCD found for SPI device %p\n",
> -			spi);
> -		return -ENODEV;
> -	}
> +	max3421_hcd = spi_get_drvdata(spi);
> +	hcd = max3421_to_hcd(max3421_hcd);
>  
>  	usb_remove_hcd(hcd);
>  
>  	spin_lock_irqsave(&max3421_hcd->lock, flags);
>  
>  	kthread_stop(max3421_hcd->spi_thread);
> -	*prev = max3421_hcd->next;
>  
>  	spin_unlock_irqrestore(&max3421_hcd->lock, flags);
>  