Messages in this thread | | | Date | Fri, 4 Mar 2022 19:25:23 +0900 | From | Takashi Sakamoto <> | Subject | Re: [PATCH] firewire: core: extend card->lock in fw_core_handle_bus_reset |
| |
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
| |