| Message ID | 1444663237-238302-10-git-send-email-john.garry@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 10/12/2015 05:20 PM, John Garry wrote: > This SAS ID is chosen as Huawei IEEE id: 001882 > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/hisi_sas/hisi_sas_init.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/scsi/hisi_sas/hisi_sas_init.c b/drivers/scsi/hisi_sas/hisi_sas_init.c > index 44fc524..c295c39 100644 > --- a/drivers/scsi/hisi_sas/hisi_sas_init.c > +++ b/drivers/scsi/hisi_sas/hisi_sas_init.c > @@ -283,6 +283,19 @@ err_out: > return NULL; > } > > +static void hisi_sas_init_add(struct hisi_hba *hisi_hba) > +{ > + u8 i; > + > + /* Huawei IEEE id (001882) */ > + for (i = 0; i < hisi_hba->n_phy; i++) > + hisi_hba->phy[i].dev_sas_addr = > + cpu_to_be64(0x5001882016072015ULL); > + Ouch. Each phy has the same SAS address? For all boards? Ever? Not sure if that's a good idea, nor even valid. It'll confuse the hell out of any SAS array. Please provide a means of having individual SAS addresses for each HBA. Cheers, Hannes
On 13/10/2015 07:12, Hannes Reinecke wrote: > On 10/12/2015 05:20 PM, John Garry wrote: >> This SAS ID is chosen as Huawei IEEE id: 001882 >> >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> drivers/scsi/hisi_sas/hisi_sas_init.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/drivers/scsi/hisi_sas/hisi_sas_init.c b/drivers/scsi/hisi_sas/hisi_sas_init.c >> index 44fc524..c295c39 100644 >> --- a/drivers/scsi/hisi_sas/hisi_sas_init.c >> +++ b/drivers/scsi/hisi_sas/hisi_sas_init.c >> @@ -283,6 +283,19 @@ err_out: >> return NULL; >> } >> >> +static void hisi_sas_init_add(struct hisi_hba *hisi_hba) >> +{ >> + u8 i; >> + >> + /* Huawei IEEE id (001882) */ >> + for (i = 0; i < hisi_hba->n_phy; i++) >> + hisi_hba->phy[i].dev_sas_addr = >> + cpu_to_be64(0x5001882016072015ULL); >> + > Ouch. Each phy has the same SAS address? > For all boards? Ever? > > Not sure if that's a good idea, nor even valid. > It'll confuse the hell out of any SAS array. > > Please provide a means of having individual SAS addresses for each HBA. > > Cheers, > > Hannes > Hello, Are you saying we should be getting the SAS address from fw with sas_request_addr() or the like? Marvell solution seems to hardcode it. thanks, John -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/13/2015 07:14 PM, John Garry wrote: > On 13/10/2015 07:12, Hannes Reinecke wrote: >> On 10/12/2015 05:20 PM, John Garry wrote: >>> This SAS ID is chosen as Huawei IEEE id: 001882 >>> >>> Signed-off-by: John Garry <john.garry@huawei.com> >>> --- >>> drivers/scsi/hisi_sas/hisi_sas_init.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_init.c >>> b/drivers/scsi/hisi_sas/hisi_sas_init.c >>> index 44fc524..c295c39 100644 >>> --- a/drivers/scsi/hisi_sas/hisi_sas_init.c >>> +++ b/drivers/scsi/hisi_sas/hisi_sas_init.c >>> @@ -283,6 +283,19 @@ err_out: >>> return NULL; >>> } >>> >>> +static void hisi_sas_init_add(struct hisi_hba *hisi_hba) >>> +{ >>> + u8 i; >>> + >>> + /* Huawei IEEE id (001882) */ >>> + for (i = 0; i < hisi_hba->n_phy; i++) >>> + hisi_hba->phy[i].dev_sas_addr = >>> + cpu_to_be64(0x5001882016072015ULL); >>> + >> Ouch. Each phy has the same SAS address? >> For all boards? Ever? >> >> Not sure if that's a good idea, nor even valid. >> It'll confuse the hell out of any SAS array. >> >> Please provide a means of having individual SAS addresses for each >> HBA. >> >> Cheers, >> >> Hannes >> > Hello, > > Are you saying we should be getting the SAS address from fw with > sas_request_addr() or the like? > > Marvell solution seems to hardcode it. > Doesn't make it any better, nor even valid. Problem is that using a hardcoded SAS address makes it impossible do any resource allocation on a SAS target array. The SAS target array is allocating / mapping the LUNs based on the SAS address, so if HBAs from different machines coming in with the same SAS address the SAS array will assume that all ports belong to the same machine, thus allowing access to all of them. And don't try to _ever_ connect these HBAs to a SAS switch; that will be even more confused. So please, if you have a chance _DO_ get the SAS address from fw. If the firmware doesn't provide it you really should get in touch with the firmware team to get you one. To clarify the situation the SAS spec states: Device names are worldwide unique names for devices within a transport protocol (see SAM-3). and: The VENDOR - SPECIFIC IDENTIFIER contains a 36-bit numeric value that is uniquely assigned by the organization associated with the company identifier in the IEEE COMPANY ID field. So as you are using the Huawei Vendor ID Huawei as a company needs to ensure uniqueness of the vendor-specific identifier (ie the last 36 bits of the SAS address). Which the above patch most patently fails to address. Cheers, Hannes
On 14/10/2015 09:40, Hannes Reinecke wrote: > On 10/13/2015 07:14 PM, John Garry wrote: >> On 13/10/2015 07:12, Hannes Reinecke wrote: >>> On 10/12/2015 05:20 PM, John Garry wrote: >>>> This SAS ID is chosen as Huawei IEEE id: 001882 >>>> >>>> Signed-off-by: John Garry <john.garry@huawei.com> >>>> --- >>>> drivers/scsi/hisi_sas/hisi_sas_init.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>>> >>>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_init.c >>>> b/drivers/scsi/hisi_sas/hisi_sas_init.c >>>> index 44fc524..c295c39 100644 >>>> --- a/drivers/scsi/hisi_sas/hisi_sas_init.c >>>> +++ b/drivers/scsi/hisi_sas/hisi_sas_init.c >>>> @@ -283,6 +283,19 @@ err_out: >>>> return NULL; >>>> } >>>> >>>> +static void hisi_sas_init_add(struct hisi_hba *hisi_hba) >>>> +{ >>>> + u8 i; >>>> + >>>> + /* Huawei IEEE id (001882) */ >>>> + for (i = 0; i < hisi_hba->n_phy; i++) >>>> + hisi_hba->phy[i].dev_sas_addr = >>>> + cpu_to_be64(0x5001882016072015ULL); >>>> + >>> Ouch. Each phy has the same SAS address? >>> For all boards? Ever? >>> >>> Not sure if that's a good idea, nor even valid. >>> It'll confuse the hell out of any SAS array. >>> >>> Please provide a means of having individual SAS addresses for each >>> HBA. >>> >>> Cheers, >>> >>> Hannes >>> >> Hello, >> >> Are you saying we should be getting the SAS address from fw with >> sas_request_addr() or the like? >> >> Marvell solution seems to hardcode it. >> > Doesn't make it any better, nor even valid. > > Problem is that using a hardcoded SAS address makes it impossible > do any resource allocation on a SAS target array. > The SAS target array is allocating / mapping the LUNs based on the > SAS address, so if HBAs from different machines coming in with the > same SAS address the SAS array will assume that all ports belong to > the same machine, thus allowing access to all of them. > > And don't try to _ever_ connect these HBAs to a SAS switch; that > will be even more confused. > > So please, if you have a chance _DO_ get the SAS address from fw. > If the firmware doesn't provide it you really should get in touch > with the firmware team to get you one. > > To clarify the situation the SAS spec states: > > Device names are worldwide unique names for devices within a > transport protocol (see SAM-3). > > and: > The VENDOR - SPECIFIC IDENTIFIER contains a 36-bit numeric value > that is uniquely assigned by the organization associated with the > company identifier in the IEEE COMPANY ID field. > > So as you are using the Huawei Vendor ID Huawei as a company needs > to ensure uniqueness of the vendor-specific identifier (ie the last > 36 bits of the SAS address). Which the above patch most patently > fails to address. > > Cheers, > > Hannes > Hi, OK, we can look at adding the ability to read the SAS HBA address from a FW image or EFI variables. Thanks, John -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 14 October 2015 16:05:21 John Garry wrote: > > OK, we can look at adding the ability to read the SAS HBA address from a > FW image or EFI variables. > The easiest way is usually to have a DT property that gets updated by the firmware. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/14/2015 11:18 PM, Arnd Bergmann wrote: > On Wednesday 14 October 2015 16:05:21 John Garry wrote: >> >> OK, we can look at adding the ability to read the SAS HBA address from a >> FW image or EFI variables. >> > > The easiest way is usually to have a DT property that gets updated > by the firmware. > Yes In net subsystem, there is mac-address. In dts, we set default mac-address, which will be modified by boot-loader, if all 0 random address will be used. mac-address = [00 00 00 00 00 00]; In driver, mac-address can be get via of_get_mac_address. Can we use the similar method here? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 15 October 2015 11:36:53 zhangfei wrote: > On 10/14/2015 11:18 PM, Arnd Bergmann wrote: > > On Wednesday 14 October 2015 16:05:21 John Garry wrote: > >> > >> OK, we can look at adding the ability to read the SAS HBA address from a > >> FW image or EFI variables. > >> > > > > The easiest way is usually to have a DT property that gets updated > > by the firmware. > > > > Yes > In net subsystem, there is mac-address. > > In dts, we set default mac-address, which will be modified by > boot-loader, if all 0 random address will be used. > mac-address = [00 00 00 00 00 00]; > > In driver, mac-address can be get via of_get_mac_address. > > Can we use the similar method here? Good idea, I think this is the best way. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/hisi_sas/hisi_sas_init.c b/drivers/scsi/hisi_sas/hisi_sas_init.c index 44fc524..c295c39 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_init.c +++ b/drivers/scsi/hisi_sas/hisi_sas_init.c @@ -283,6 +283,19 @@ err_out: return NULL; } +static void hisi_sas_init_add(struct hisi_hba *hisi_hba) +{ + u8 i; + + /* Huawei IEEE id (001882) */ + for (i = 0; i < hisi_hba->n_phy; i++) + hisi_hba->phy[i].dev_sas_addr = + cpu_to_be64(0x5001882016072015ULL); + + memcpy(hisi_hba->sas_addr, &hisi_hba->phy[0].dev_sas_addr, + SAS_ADDR_SIZE); +} + static int hisi_sas_probe(struct platform_device *pdev) { struct Scsi_Host *shost; @@ -339,6 +352,8 @@ static int hisi_sas_probe(struct platform_device *pdev) sha->sas_phy[i] = &hisi_hba->phy[i].sas_phy; sha->sas_port[i] = &hisi_hba->port[i].sas_port; } + + hisi_sas_init_add(hisi_hba); rc = scsi_add_host(shost, &pdev->dev); if (rc) goto err_out_ha;
This SAS ID is chosen as Huawei IEEE id: 001882 Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/hisi_sas/hisi_sas_init.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)