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@xxxxxxxxxx>
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@xxxxxxxxxxxxxx>
> ---
> 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);
>
