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

[Damien Le Moal <dlemoal@kernel.org>]

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).

Cheers.


-- 
Damien Le Moal
Western Digital Research