Re: [PATCH] Revert "scsi: pm80xx: Do not use libsas port ID"

[Igor Pylypiv <ipylypiv@google.com>]

On Tue, Jul 22, 2025 at 01:25:47PM +0900, Damien Le Moal wrote:
> On 7/22/25 10:28 AM, Damien Le Moal wrote:
> > On 7/19/25 7:30 AM, Igor Pylypiv wrote:
> >> Hey Niklas,
> >>
> >> Could you try the following fix with your expander setup, please?
> >> The fix assumes the problematic patch is not yet revered.
> >>
> >> $ git diff
> >> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> >> index f7067878b34f..cd9513c23c71 100644
> >> --- a/drivers/scsi/pm8001/pm8001_sas.c
> >> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> >> @@ -503,7 +503,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
> >>         spin_lock_irqsave(&pm8001_ha->lock, flags);
> >>  
> >>         pm8001_dev = dev->lldd_dev;
> >> -       port = pm8001_ha->phy[pm8001_dev->attached_phy].port;
> >> +       port = dev->port->lldd_port;
> >>  
> >>         if (!internal_abort &&
> >>             (DEV_IS_GONE(pm8001_dev) || !port || !port->port_attached)) {
> > 
> > Igor,
> > 
> > I tested this, or rather, a variation of it that clean things up at the same time:
> > 
> > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> > index f7067878b34f..753c09363cbb 100644
> > --- a/drivers/scsi/pm8001/pm8001_sas.c
> > +++ b/drivers/scsi/pm8001/pm8001_sas.c
> > @@ -477,7 +477,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t
> > gfp_flags)
> >         struct pm8001_device *pm8001_dev = dev->lldd_dev;
> >         bool internal_abort = sas_is_internal_abort(task);
> >         struct pm8001_hba_info *pm8001_ha;
> > -       struct pm8001_port *port = NULL;
> > +       struct pm8001_port *port;
> >         struct pm8001_ccb_info *ccb;
> >         unsigned long flags;
> >         u32 n_elem = 0;
> > @@ -502,8 +502,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t
> > gfp_flags)
> > 
> >         spin_lock_irqsave(&pm8001_ha->lock, flags);
> > 
> > -       pm8001_dev = dev->lldd_dev;
> > -       port = pm8001_ha->phy[pm8001_dev->attached_phy].port;
> > +       port = dev->port->lldd_port;
> > 
> >         if (!internal_abort &&
> >             (DEV_IS_GONE(pm8001_dev) || !port || !port->port_attached)) {
> > 
> > 
> > And it works, I can see the drives in the enclosure behind the expander.
> > Care to send a proper path ?
> > 
> > I think this needs more testing though, especially special cases like yanking
> > the SAS cable and doing device hotplug/unplug. Will do that later today.
> 
> So I did that. And things are not pretty... Even a simple "rmmod pm80xx"
> crashes the kernel on a bad pointer dereference (invalid port address). Same if
> I hot-unplug drives from the enclosure. But that happens even with only Niklas
> revert patch applied. So I think that is unrelated to this change.
> 
> That said, I will dig further to understand how the port pointers become
> invalid, and make sure this change is OK. Note that there are no issues that I
> can see when there is no expander (drives directly attached to the HBA).

Thank you for testing, Damien!

Just guessing, would defining the lldd_port_deformed() callback help?
The callback can set lldd_port to NULL if the problem is due to a dangling
lldd_port pointer.

Thanks,
Igor

> 
> Cheers.
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research