Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 4 Mar 2022 19:25:23 +0900
From:   Takashi Sakamoto <o-takashi@sakamocchi.jp>
To:     Niels Dossche <dossche.niels@gmail.com>
Cc:     linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] firewire: core: extend card->lock in
 fw_core_handle_bus_reset
Message-ID: <YiHpEx/1LaGuzEMC@workstation>
Mail-Followup-To: Niels Dossche <dossche.niels@gmail.com>,
        linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
References: <20220303183038.54126-1-dossche.niels@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220303183038.54126-1-dossche.niels@gmail.com>
Precedence: bulk

Hi,

On Thu, Mar 03, 2022 at 07:30:38PM +0100, Niels Dossche wrote:
> card->local_node and card->bm_retries are both always accessed under
> card->lock.
> fw_core_handle_bus_reset has a check whose condition depends on
> card->local_node and whose body writes to card->bm_retries.
> Both of these accesses are not under card->lock. Move the lock acquiring
> of card->lock to before this check such that these accesses do happen
> when card->lock is held.
> fw_destroy_nodes is called inside the check.
> Since fw_destroy_nodes already acquires card->lock inside its function
> body, move this out to the callsites of fw_destroy_nodes.
> Also add a comment to indicate which locking is necessary when calling
> fw_destroy_nodes.
> 
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
>  drivers/firewire/core-card.c     | 3 +++
>  drivers/firewire/core-topology.c | 9 +++------
>  2 files changed, 6 insertions(+), 6 deletions(-)
 
It looks good to me and is preferable as Niels said.

Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>


I think the patch is for minor code refactoring rather than bug fix
since it is unlikely that the rate condition occurs. The bus_reset_work
is synchronously canceled in the beginning of pci_remove().

```
pci_remove()
->cancel_work_sync(&ohci->bus_reset_work);
->fw_core_remove_card(&ohci->card);
->software_reset(ohci);
->free_irq(dev->irq, ohci);
```

But I have no confidence that the above calls are done in the order by
processor...

> diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
> index 54be88167c60..f3b3953cac83 100644
> --- a/drivers/firewire/core-card.c
> +++ b/drivers/firewire/core-card.c
> @@ -668,6 +668,7 @@ EXPORT_SYMBOL_GPL(fw_card_release);
>  void fw_core_remove_card(struct fw_card *card)
>  {
>  	struct fw_card_driver dummy_driver = dummy_driver_template;
> +	unsigned long flags;
>  
>  	card->driver->update_phy_reg(card, 4,
>  				     PHY_LINK_ACTIVE | PHY_CONTENDER, 0);
> @@ -682,7 +683,9 @@ void fw_core_remove_card(struct fw_card *card)
>  	dummy_driver.stop_iso		= card->driver->stop_iso;
>  	card->driver = &dummy_driver;
>  
> +	spin_lock_irqsave(&card->lock, flags);
>  	fw_destroy_nodes(card);
> +	spin_unlock_irqrestore(&card->lock, flags);
>  
>  	/* Wait for all users, especially device workqueue jobs, to finish. */
>  	fw_card_put(card);
> diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
> index b63d55f5ebd3..f40c81534381 100644
> --- a/drivers/firewire/core-topology.c
> +++ b/drivers/firewire/core-topology.c
> @@ -375,16 +375,13 @@ static void report_found_node(struct fw_card *card,
>  	card->bm_retries = 0;
>  }
>  
> +/* Must be called with card->lock held */
>  void fw_destroy_nodes(struct fw_card *card)
>  {
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&card->lock, flags);
>  	card->color++;
>  	if (card->local_node != NULL)
>  		for_each_fw_node(card, card->local_node, report_lost_node);
>  	card->local_node = NULL;
> -	spin_unlock_irqrestore(&card->lock, flags);
>  }
>  
>  static void move_tree(struct fw_node *node0, struct fw_node *node1, int port)
> @@ -510,6 +507,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
>  	struct fw_node *local_node;
>  	unsigned long flags;
>  
> +	spin_lock_irqsave(&card->lock, flags);
> +
>  	/*
>  	 * If the selfID buffer is not the immediate successor of the
>  	 * previously processed one, we cannot reliably compare the
> @@ -521,8 +520,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
>  		card->bm_retries = 0;
>  	}
>  
> -	spin_lock_irqsave(&card->lock, flags);
> -
>  	card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated;
>  	card->node_id = node_id;
>  	/*
> -- 
> 2.35.1


Regards

Takashi Sakamoto