Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <cf98eb9f-ac42-4d9b-9cf3-3085f6fc0cda@oracle.com>
Date:   Wed, 29 Nov 2023 12:54:33 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4] scsi: libsas: Fix the failure of adding phy with
 zero-address to port
To:     yangxingui <yangxingui@huawei.com>, yanaijie@huawei.com,
        jejb@linux.ibm.com, martin.petersen@oracle.com,
        damien.lemoal@opensource.wdc.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, prime.zeng@hisilicon.com,
        kangfenglong@huawei.com, chenxiang66@hisilicon.com
References: <20231117090001.35840-1-yangxingui@huawei.com>
 <32c42e1e-0399-4af4-a5ed-6a257e300fe8@oracle.com>
 <307d251f-ff49-5d8f-1f8e-aed314256732@huawei.com>
 <a13f0419-c4ef-4b8b-9757-7cf7cea32458@oracle.com>
 <baacad33-f568-6151-75a2-dfc09caf2a81@huawei.com>
Content-Language: en-US
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <baacad33-f568-6151-75a2-dfc09caf2a81@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MVFLeUtlY2JNNmtFek5vUmRiSGw1WkNiRFRyTm12OHY3MkRZekxGRDVIVU5Y?=
 =?utf-8?B?eTdyQzd5cUk5UG8zMmFnNjRPRko5NlcvVTByS1JxcWpwSVZPUHBaemRxc2tn?=
 =?utf-8?B?Qk1FMlFidHhKbHUzOTZBL3p0TlUwaGloZi9IdjRBbldrV1BVOXJpZkdiYkZN?=
 =?utf-8?B?OFphcGF1MjE0b0lYWnFoSjQwZ1QrWDVpVzBjNStwcTdYMitRMXI0a1plZkZu?=
 =?utf-8?B?L2dlRzQvTFNYaDNGSE0yc1R1QXJ0aWE2RUg1UmtuL2trWnFVSW5iSURYazN2?=
 =?utf-8?B?YThYTUpnRm00SHNoVGx2MFVtdmF6TkorTjZFcGVQQTkzQW9KaUJSZ0NTQitB?=
 =?utf-8?B?bUJ0cUt1ejVQNmxOZ1UzalQ0Uk9hSWtSK2lyRi9GRXZqeG8va2M5S1VLUG9C?=
 =?utf-8?B?c1BScm9aVjBZekVjUlB6WG9Eb25UY2NNMHRqSkNxckJUaDhwNDliN1lhWXFr?=
 =?utf-8?B?NlRVZGozeUdZOUQ1em5oWU54MGNGVzVTWk9tc1duS2JFUnJsMjRIVU9pV2pk?=
 =?utf-8?B?dEoxQ1FFRUI3T2Q3djFRVlhmU0ptdTFzUFJJZXJ4ZmtaK0tiZzloWDNzZ3Nw?=
 =?utf-8?B?dFVZZ2FLWW9INk9YanlhMTdYSktrN2VLUnQwL1F0SDdha0NaQVhzWTd2RGVO?=
 =?utf-8?B?RkVISFVmTVVQRjRvWmgxOEdIbE1sN1VXdGVSeXNLSkxlWm1XUFZNdFFaMFlU?=
 =?utf-8?B?cVJFNjBhUUN6Y2MyMmhPQk5XdTRHSmRidDJGcndMMEVaZWpnUzFTSnNXVnVQ?=
 =?utf-8?B?cnNNQklyWWdPNHU1YzZsMk5WdGhaK1JOem0rY1NHN013ZnZCcmZkWGovYmI1?=
 =?utf-8?B?NVpFZldrSld4dlVkYnFtclZieW05RSt1T2o5Z0t2Yzl5MVRpWSs1eUNFUUdK?=
 =?utf-8?B?QXFGKzRWVU1tY3lVdDlnTC8rd2hTTE80YWxRTmplWk0vUjFGQzRwY2R0Wnpa?=
 =?utf-8?B?VUVSelJqbHJOckE1NXlkQzl6Wit0WWlMQXBaTFdXNXM3a2E1QnNQVk1BVnV4?=
 =?utf-8?B?ODZPVUZqUmNnbStwcXVyNHoxZDQ2WnlqaFk5c3k1eE80cWlURFgzanVQbnNs?=
 =?utf-8?B?bUlUNVpZVldEdS9vMDBmRC92bEh0d29JSloycFg4MjRPTm1tK3VvOGhXQi9N?=
 =?utf-8?B?SHVSekJBRzF0QzQxeWVWWFF5bmozMUM2azVUcituMFZ1S04ybk5Neitzd1FG?=
 =?utf-8?B?WTcyKy9CZkJ6VDBMNG5wanRreTRreS92V1owMjM3ZUtoZHA4bkZBVEJTQkIy?=
 =?utf-8?B?VU54aHlmWHgwd3loVGpIRytmVG5XRnVERVA2MzRvRG5tSEFsT2dFTzM5TWhi?=
 =?utf-8?B?TFNYNmo5MEh1Mng2TUFQK1l2RWFEakRXSDJmdDkwa3gzbXhwU0F1Rk5VMWNu?=
 =?utf-8?B?UkRJUEYyc0V6OHZrRHlRZzk1ajlMOWRCZ3VvTDJPNzNTeVVIRXlrbW8ya1pz?=
 =?utf-8?B?cWt4dFlLMjY4VTdxb2R1MWU5aXlRdjhCYjJScE15YnJtSHFGZTNvRmoySzlT?=
 =?utf-8?B?Z2lwSlZqUVNGbnN5Zll1TmtpT2tZaFhhOVhWOXVOOUhNUU9rdTNuVjFOUWxz?=
 =?utf-8?B?eFk3c3hCd0Vtd2RiWDhZVS9CUEM4YzVYS2VrWVVxaTFxMThmaGdJRkU4eDJX?=
 =?utf-8?B?d25ZbW5PNnRTbHE5YmZXdlMzaXI3U2YzR29tYTlZc091dE5xNzZTK1lUSFR2?=
 =?utf-8?B?d1RjckxQcmZid1k5bm82NUJrTGJhTC9OT21CVDVSUFhQTE85K0NTSy9xUU1B?=
 =?utf-8?B?eWpDSlhRL1F0N2lGdUMvZFBnNkNEaTNIQUxxUUdEek9WK2pFWUxDWXEvZm1X?=
 =?utf-8?B?ZHZRV2xPWjFxbzhBaHJKUVo3d0U4WjVzN21Ga2hieFZlV1plRHc3UUtlUDJE?=
 =?utf-8?B?anpCdGJiaGVkM1lVeDNzU0txTjhzZ2dweS9WK29HQVdXOTF2ajdMZTZ5WkRj?=
 =?utf-8?B?b3F2Y0xvcXpTdVBENytzcDkrYUZ6YUNJMnRlbm84NExNSVhkVXhDdVJuOFd6?=
 =?utf-8?B?c05ua3k1cCsxMEJsRGhveWFrOW1DNjg5NDlyNzZmR3VTdlhkMmNnVEpPczFG?=
 =?utf-8?B?Y2R2Y043QVVBelhIMGdRTWZnWXk3NmhxdVFEK09sYzFFTEtlV0VJZEdkbmNB?=
 =?utf-8?Q?bGSpQjGAvmgl324y+icdw3jkE?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: jMcLH/M+edDNFiqMJ8fBfa0Zr1X68V/fQdQRp+d3QU/oIcuBeG1IM6iDExOk1sLhJovNIhkCEJ/YouPSSHyF4DMXm+qddJ7On3ck4UkFlCKXm3PS6Tiv05AuezAx+HLc3ImVRvfJnVJegsejIpujyp63wczNEAa9/PTYoKzEC7aKxaVuJVQmJVnjR8taUNClxbo2GfvAfGwwCOnxDrY0hTkF+Xwg1PdMgv+y6oaRK498KL6k/skX2zZlAWgHb86H0Wh8Nw61MIiS6b+XuD0GGSIRhL1agsh3WsMD+ccV/CBwxfNrZQRVqf8NTl1fEM/eEg+vqWBYXv1st5ocCViSnDHkBQOak38q7n6MEoqknvU2uD4MhxcR5fOnaugbZsHz0krjihmmgfXEx04BUGVRkZy3S5JIGgqG3jhZVPC8TQcJVW9JzNPFV8tCPTWZq+SV0JFEfUHISdRmSAdtwcsF5+Z/dFld440nVYm3Gyoho3WyTssx4NGVQXsrL7DxlpwiOK6OnO4KXqtw+IESJNEYl0daiiZuUu+cpXfxTO/HkH6JOZw7K0MeN5QLp2Sz4i5js5RcvZHTNdRHrOJpc3Z+SEhZ1FD0jpxqxyoomNvxbLAWardQwlsAscsACzlvEslqi4999nXX3DQ7fQakTV+JC7Sv8u0U9ZzIG7vSKCPmzHpIX3QKGgKT/68xUyrzy8X/UXvC3sVKs1VNjVFo9uFezWbmvzAy6y6AJLkyINj7UM66XWqxENL68x7DXu6MUy7nWaCAGknsZ1gLca+s1p9u7x223dO3h5/yH5UYnUuj/GwFyBzmdpKV5hRWrvCK5hhDvaDUw6Y6OcOH9OmIJ3fQr4jSqqb6zruxJOAkdyfr36QbnZsRAwVxC+VktmtCQWqIFZUe65tI4UX0Ni7HFo6c3L8PJ6Xy5jlk9uNxWaQwbQ8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 45b2ae20-7b6a-4604-6181-08dbf0da57f0
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Nov 2023 12:54:37.4229
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: b91NT0O5lr5TjX1mazRVM8b13zZNekidhwUPCyGazB/069KFJI4nHO/LMdp7NloJgn3SR6u6dN+c5mbSouHdbw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB4997
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-11-29_09,2023-11-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0
 suspectscore=0 mlxscore=0 spamscore=0 adultscore=0 mlxlogscore=999
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311060000 definitions=main-2311290097
X-Proofpoint-GUID: ffZzoPO3XdhWwlAr0GS40EhFBUz7K72K
X-Proofpoint-ORIG-GUID: ffZzoPO3XdhWwlAr0GS40EhFBUz7K72K
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 29 Nov 2023 04:55:13 -0800 (PST)

On 28/11/2023 03:45, yangxingui wrote:
> 
> On 2023/11/28 3:28, John Garry wrote:
>> On 24/11/2023 02:27, yangxingui wrote:
>>>> We already do this in sas_ex_join_wide_port(), right?
>>> No, If the addr of ex_phy matches dev->parent, 
>>> sas_ex_join_wide_port() will not be called, but sas_add_parent_port() 
>>> will be called  as follows:
>>> static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
>>> {
>>>          struct expander_device *ex = &dev->ex_dev;
>>>          struct ex_phy *ex_phy = &ex->ex_phy[phy_id];
>>>          struct domain_device *child = NULL;
>>>          int res = 0;
>>>
>>>      <...>
>>>          /* Parent and domain coherency */
>>>          if (!dev->parent && sas_phy_match_port_addr(dev->port, 
>>> ex_phy)) {
>>>                  sas_add_parent_port(dev, phy_id);
>>>                  return 0;
>>>          }
>>>          if (dev->parent && sas_phy_match_dev_addr(dev->parent, 
>>> ex_phy)) {
>>>                  sas_add_parent_port(dev, phy_id);
>>>                  if (ex_phy->routing_attr == TABLE_ROUTING)
>>>                          sas_configure_phy(dev, phy_id, 
>>> dev->port->sas_addr, 1);
>>>                  return 0;
>>>          }
>>>      <...>
>>> }
>>>
>>>>
>>>> I am not saying that what we do now does not have a problem - I am 
>>>> just trying to understand what currently happens
>>>
>>> ok, because ex_phy->port is not set when calling 
>>> sas_add_parent_port(), when deleting phy from the parent wide port, 
>>> it is not removed from the phy_list of the parent wide port as follows:
>>> static void sas_unregister_devs_sas_addr(struct domain_device *parent,
>>>                                           int phy_id, bool last)
>>> {
>>>      <...>
>>>      // Since ex_phy->port is not set, this branch will not be enter
>>
>> But then how does this ever work? It is because we follow path 
>> sas_rediscover_dev() -> sas_discover_new() -> 
>> sas_ex_discover_devices() -> sas_ex_discover_dev() -> 
>> sas_add_parent_port(), and not sas_rediscover_dev() -> 
>> sas_discover_new() -> sas_ex_join_wide_port()? If so, is that because 
>> ephy->sas_attached_phy == 0 in sas_discover_new() -> 
>> sas_ex_join_wide_port() and it fails?
>>
>> BTW, about something mentioned earlier - adding the phy19 with SAS_ADDR 
> 
> Yes,
> For phy19, when the phy is attached and added to the parent wide port, 
> the path is:
> sas_rediscover()
>      ->sas_discover_new()
>          ->sas_ex_discover_devices()
>              ->sas_ex_discover_dev()
>                  -> sas_add_parent_port().

ok, so then the change to set ex_phy->port = ex->parent_port looks ok. 
Maybe we can put this in a helper with the sas_port_add_phy() call, as 
it is duplicated in sas_ex_join_wide_port()

Do we also need to set ex_phy->phy_state (like sas_ex_join_wide_port())?

> And the path called when it is removed from parent wide port is:
> sas_rediscover()
>      ->sas_unregister_devs_sas_addr() // The sas address of phy19 
> becomes 0. Since ex_phy->port is NULL, phy19 is not removed from the 
> parent wide port's phy_list.
> 
> For phy0, it is connected to a new sata device.
> sas_rediscover()
>      ->sas_discover_new()->sas_ex_phy_discover()
>                              ->sas_ex_phy_discover_helper()
>                                  ->sas_set_ex_phy() // The device type 
> is stp. Since the linkrate is 5 and less than 1.5G, sas_address is set 
> to 0.

Then when we get the proper linkrate later, will we then rediscover and 
set the proper SAS address? I am just wondering if this change is really 
required?

BTW, Even with the change to set ex_phy->port = ex->parent_port, are we 
still joining the host-attached expander phy (19) to a port with SAS 
address == 0?

>                          ->sas_ex_discover_devices()
>                              ->sas_ex_discover_dev()
>                                  ->sas_ex_discover_end_dev()
>                                      ->sas_port_alloc() // Create 
> port-7:7:0
>                                      ->sas_ex_get_linkrate()
>                                          ->sas_port_add_phy() // Try 
> adding phy19 to port->7:7:0, triggering BUG()

Thanks,
John