Thanks for the notice!
Yes, removing the lock acquisition inside hisi_sas_port_notify_formed()
can avoid the deadlock problem by the way.
Best Regards,
Chengfeng
chenxiang (M) <chenxiang66@xxxxxxxxxxxxx> 于2023年7月4日周二 13:55写道：
>
> Hi,
>
>
> 在 2023/6/28 星期三 23:30, Chengfeng Ye 写道:
> > As &hisi_hba->lock is acquired by hard irq int_abnormal_v1_hw(),
> > other acquisition of the same lock under process context should
> > disable irq, otherwise deadlock could happen if the
> > irq preempt the execution while the lock is held in process context
> > on the same CPU.
> >
> > [Interrupt]: int_abnormal_v1_hw()
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c:1447
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:2050
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1079
> > -->spin_lock_irqsave(&hisi_hba->lock, flags);
> >
> > [Process Context]: hisi_sas_clear_nexus_ha()
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1932
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1135
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1116
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1105
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:166
> > -->spin_lock(&hisi_hba->lock);
> >
> > [Process Context]: hisi_sas_dev_found()
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:665
> > -->spin_lock(&hisi_hba->lock);
> >
> > [Process Context]: hisi_sas_queue_command()
> > -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:188
> > -->spin_lock(&hisi_hba->lock);
> >
> > This flaw was found by an experimental static analysis tool I am
> > developing for irq-related deadlock, which reported the above
> > warning when analyzing the linux kernel 6.4-rc7 release.
> >
> > The tentative patch fix the potential deadlock by spin_lock_irqsave().
>
> Thank you for reporting it.
> But we consider about removing hisi_hba->lock in function
> hisi_sas_port_notify_formed()
> which is called by int_abnormal_v1_hw(), as we think it is not necessary
> to add hisi_hba->lock in this function.
> So please ignore it and still thank you for pointing out the issue.
>
> Thanks,
> Shawn
>
> >
> > Signed-off-by: Chengfeng Ye <dg573847474@xxxxxxxxx>
> > ---
> > drivers/scsi/hisi_sas/hisi_sas_main.c | 17 ++++++++++-------
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> > index 412431c901a7..47c5062a732e 100644
> > --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> > +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> > @@ -161,11 +161,12 @@ static void hisi_sas_slot_index_clear(struct hisi_hba *hisi_hba, int slot_idx)
> >
> > static void hisi_sas_slot_index_free(struct hisi_hba *hisi_hba, int slot_idx)
> > {
> > + unsigned long flags;
> > if (hisi_hba->hw->slot_index_alloc ||
> > slot_idx < HISI_SAS_RESERVED_IPTT) {
> > - spin_lock(&hisi_hba->lock);
> > + spin_lock_irqsave(&hisi_hba->lock, flags);
> > hisi_sas_slot_index_clear(hisi_hba, slot_idx);
> > - spin_unlock(&hisi_hba->lock);
> > + spin_unlock_irqrestore(&hisi_hba->lock, flags);
> > }
> > }
> >
> > @@ -181,11 +182,12 @@ static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
> > {
> > int index;
> > void *bitmap = hisi_hba->slot_index_tags;
> > + unsigned long flags;
> >
> > if (rq)
> > return rq->tag + HISI_SAS_RESERVED_IPTT;
> >
> > - spin_lock(&hisi_hba->lock);
> > + spin_lock_irqsave(&hisi_hba->lock, flags);
> > index = find_next_zero_bit(bitmap, HISI_SAS_RESERVED_IPTT,
> > hisi_hba->last_slot_index + 1);
> > if (index >= HISI_SAS_RESERVED_IPTT) {
> > @@ -193,13 +195,13 @@ static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
> > HISI_SAS_RESERVED_IPTT,
> > 0);
> > if (index >= HISI_SAS_RESERVED_IPTT) {
> > - spin_unlock(&hisi_hba->lock);
> > + spin_unlock_irqrestore(&hisi_hba->lock, flags);
> > return -SAS_QUEUE_FULL;
> > }
> > }
> > hisi_sas_slot_index_set(hisi_hba, index);
> > hisi_hba->last_slot_index = index;
> > - spin_unlock(&hisi_hba->lock);
> > + spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >
> > return index;
> > }
> > @@ -658,11 +660,12 @@ static struct hisi_sas_device *hisi_sas_alloc_dev(struct domain_device *device)
> > {
> > struct hisi_hba *hisi_hba = dev_to_hisi_hba(device);
> > struct hisi_sas_device *sas_dev = NULL;
> > + unsigned long flags;
> > int last = hisi_hba->last_dev_id;
> > int first = (hisi_hba->last_dev_id + 1) % HISI_SAS_MAX_DEVICES;
> > int i;
> >
> > - spin_lock(&hisi_hba->lock);
> > + spin_lock_irqsave(&hisi_hba->lock, flags);
> > for (i = first; i != last; i %= HISI_SAS_MAX_DEVICES) {
> > if (hisi_hba->devices[i].dev_type == SAS_PHY_UNUSED) {
> > int queue = i % hisi_hba->queue_count;
> > @@ -682,7 +685,7 @@ static struct hisi_sas_device *hisi_sas_alloc_dev(struct domain_device *device)
> > i++;
> > }
> > hisi_hba->last_dev_id = i;
> > - spin_unlock(&hisi_hba->lock);
> > + spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >
> > return sas_dev;
> > }
>
