Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <960e2aac-1769-6037-dd77-58999313fefc@kernel.org>
Date:   Mon, 22 May 2023 18:44:13 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2] ata: libata-scsi: Fix get identity data failed
Content-Language: en-US
To:     Jason Yan <yanaijie@huawei.com>,
        yangxingui <yangxingui@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, john.g.garry@oracle.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, prime.zeng@hisilicon.com,
        kangfenglong@huawei.com
References: <20230505025712.19438-1-yangxingui@huawei.com>
 <291f1d97-9195-45ac-8e12-058f5c797277@kernel.org>
 <02d36ee9-cdad-454d-d822-95442d7bd67b@huawei.com>
 <f4ba7a92-1f00-c254-d196-7d21fe14dee2@kernel.org>
 <938d6b5b-0271-977d-f046-5fd70d29b3ca@huawei.com>
 <a5c2e157-aaf7-1300-3fbb-1300ac216cee@kernel.org>
 <68953040-1622-254b-f6f8-b279eccacfb1@kernel.org>
 <c88dcbc3-d530-3e9e-f674-a2fe64ad5fdc@huawei.com>
From:   Damien Le Moal <dlemoal@kernel.org>
Organization: Western Digital Research
In-Reply-To: <c88dcbc3-d530-3e9e-f674-a2fe64ad5fdc@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/22/23 17:00, Jason Yan wrote:
>> OK, so the issue is that __ata_scsi_find_dev() calls ata_find_dev() with devno
>> == scsidev->id. This leads to devno being 0, 1, 2 and 3 for connected drives
>> sdd, sd1, sdf and sdg, as shown by lsscsi. However, each drive has its own
>> port+link, with the link for each one having  ata_link_max_devices() == 1, so
>> ata_find_dev() works only for the first drive with scsidev->id == 0 and fails
>> for the others. A naive fix would be this:
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 7bb12deab70c..e4d6f17d7ccc 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2718,7 +2718,7 @@ static struct ata_device *__ata_scsi_find_dev(struct
>> ata_port *ap,
>>          if (!sata_pmp_attached(ap)) {
>>                  if (unlikely(scsidev->channel || scsidev->lun))
>>                          return NULL;
>> -               devno = scsidev->id;
>> +               devno = 0;
>>          } else {
>>                  if (unlikely(scsidev->id || scsidev->lun))
>>                          return NULL;
>>
>> And running this on my setup, it works. This makes libsas added ports/devices
>> look like AHCI ones, where all devices have ID 0 for the !pmp case.
>>
>> However, I am not sure this would be OK for all setups...
>>
>> John,
>>
>> Any idea if there is any cases where libsas managed drives would endup not being
>> correctly identified by this change ? As long as a device always has its own
>> port, I do not see any issue. But is there a case where we could have multiple
>> devices on the same port ? Per libata, max is 2, and that is only for the IDE
>> master/slave case. Otherwise, it is always 1.
>>
> 
> AFAIK, libsas does not support multiple devices on the same port. So 
> this change is ok for libsas.

Yes, for libsas it is OK. But as is, it will break master+slave IDE setups... So
the fix needs to be finer than this.

> 
>> Not that looking at the pmp case, I am not confident at all that the
>> identification is correct for libsas. But I do not think that anyone would ever
>> connect a pmp box to a libsas HBA...
>>
> 
> libsas's does not support pmp either, and I do not see any future plans 
> to support pmp.

Good. Dealing with that one is always painful.

> So the above change (needs a ATA_FLAG_SAS_HOST check) looks good to me.

Yes, this flag check is needed to avoid breaking IDE/pata.

> It's better to make libsas behave as other ata drivers so that we can 
> drop the ATA_FLAG_SAS_HOST check. But this need tons of work for libsas.

Yes, getting rid of this special casing with this flag would be really nice. It
should not be needed. I will try to write a proper fix not using it for now, to
facilitate removing the flag later.


-- 
Damien Le Moal
Western Digital Research