diff mbox series

[v7,5/7] scsi: hisi_sas: Add libsas SATA sysfs attributes group

Message ID 20240306012226.3398927-6-ipylypiv@google.com
State New
Headers show
Series NCQ Priority sysfs sttributes for libsas | expand

Commit Message

Igor Pylypiv March 6, 2024, 1:22 a.m. UTC 
The added sysfs attributes group enables the configuration of NCQ Priority
feature for HBAs that rely on libsas to manage SATA devices.

Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++++++
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++++++
 2 files changed, 12 insertions(+)

Comments

Niklas Cassel March 6, 2024, 10:55 a.m. UTC | #1 
On Tue, Mar 05, 2024 at 05:22:24PM -0800, Igor Pylypiv wrote:
> The added sysfs attributes group enables the configuration of NCQ Priority
> feature for HBAs that rely on libsas to manage SATA devices.
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Jason Yan <yanaijie@huawei.com>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++++++
>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++++++

Is there a reason why you didn't patch:
drivers/scsi/hisi_sas/hisi_sas_v1_hw.c ?


>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> index 73b378837da7..b5d379ebe05d 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> @@ -3544,6 +3544,11 @@ static struct attribute *host_v2_hw_attrs[] = {
>  
>  ATTRIBUTE_GROUPS(host_v2_hw);
>  
> +static const struct attribute_group *sdev_groups_v2_hw[] = {
> +	&sas_ata_sdev_attr_group,
> +	NULL
> +};
> +
>  static void map_queues_v2_hw(struct Scsi_Host *shost)
>  {
>  	struct hisi_hba *hisi_hba = shost_priv(shost);
> @@ -3585,6 +3590,7 @@ static const struct scsi_host_template sht_v2_hw = {
>  	.compat_ioctl		= sas_ioctl,
>  #endif
>  	.shost_groups		= host_v2_hw_groups,
> +	.sdev_groups		= sdev_groups_v2_hw,
>  	.host_reset		= hisi_sas_host_reset,
>  	.map_queues		= map_queues_v2_hw,
>  	.host_tagset		= 1,
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index b56fbc61a15a..9b69ea16a1e6 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -2929,6 +2929,11 @@ static struct attribute *host_v3_hw_attrs[] = {
>  
>  ATTRIBUTE_GROUPS(host_v3_hw);
>  
> +static const struct attribute_group *sdev_groups_v3_hw[] = {
> +	&sas_ata_sdev_attr_group,
> +	NULL
> +};
> +
>  #define HISI_SAS_DEBUGFS_REG(x) {#x, x}
>  
>  struct hisi_sas_debugfs_reg_lu {
> @@ -3340,6 +3345,7 @@ static const struct scsi_host_template sht_v3_hw = {
>  	.compat_ioctl		= sas_ioctl,
>  #endif
>  	.shost_groups		= host_v3_hw_groups,
> +	.sdev_groups		= sdev_groups_v3_hw,
>  	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,
>  	.host_reset             = hisi_sas_host_reset,
>  	.host_tagset		= 1,
> -- 
> 2.44.0.278.ge034bb2e1d-goog
>
Igor Pylypiv March 6, 2024, 8:56 p.m. UTC | #2 
On Wed, Mar 06, 2024 at 11:55:33AM +0100, Niklas Cassel wrote:
> On Tue, Mar 05, 2024 at 05:22:24PM -0800, Igor Pylypiv wrote:
> > The added sysfs attributes group enables the configuration of NCQ Priority
> > feature for HBAs that rely on libsas to manage SATA devices.
> > 
> > Reviewed-by: John Garry <john.g.garry@oracle.com>
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Reviewed-by: Jason Yan <yanaijie@huawei.com>
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++++++
> >  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++++++
> 
> Is there a reason why you didn't patch:
> drivers/scsi/hisi_sas/hisi_sas_v1_hw.c ?
> 

I originally patched hisi_sas_v1_hw.c as well. John Garry pointed out
that v1 HW doesn't support SATA so I dropped the change.

> 
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> > index 73b378837da7..b5d379ebe05d 100644
> > --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> > +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> > @@ -3544,6 +3544,11 @@ static struct attribute *host_v2_hw_attrs[] = {
> >  
> >  ATTRIBUTE_GROUPS(host_v2_hw);
> >  
> > +static const struct attribute_group *sdev_groups_v2_hw[] = {
> > +	&sas_ata_sdev_attr_group,
> > +	NULL
> > +};
> > +
> >  static void map_queues_v2_hw(struct Scsi_Host *shost)
> >  {
> >  	struct hisi_hba *hisi_hba = shost_priv(shost);
> > @@ -3585,6 +3590,7 @@ static const struct scsi_host_template sht_v2_hw = {
> >  	.compat_ioctl		= sas_ioctl,
> >  #endif
> >  	.shost_groups		= host_v2_hw_groups,
> > +	.sdev_groups		= sdev_groups_v2_hw,
> >  	.host_reset		= hisi_sas_host_reset,
> >  	.map_queues		= map_queues_v2_hw,
> >  	.host_tagset		= 1,
> > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> > index b56fbc61a15a..9b69ea16a1e6 100644
> > --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> > +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> > @@ -2929,6 +2929,11 @@ static struct attribute *host_v3_hw_attrs[] = {
> >  
> >  ATTRIBUTE_GROUPS(host_v3_hw);
> >  
> > +static const struct attribute_group *sdev_groups_v3_hw[] = {
> > +	&sas_ata_sdev_attr_group,
> > +	NULL
> > +};
> > +
> >  #define HISI_SAS_DEBUGFS_REG(x) {#x, x}
> >  
> >  struct hisi_sas_debugfs_reg_lu {
> > @@ -3340,6 +3345,7 @@ static const struct scsi_host_template sht_v3_hw = {
> >  	.compat_ioctl		= sas_ioctl,
> >  #endif
> >  	.shost_groups		= host_v3_hw_groups,
> > +	.sdev_groups		= sdev_groups_v3_hw,
> >  	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,
> >  	.host_reset             = hisi_sas_host_reset,
> >  	.host_tagset		= 1,
> > -- 
> > 2.44.0.278.ge034bb2e1d-goog
> >
John Garry March 7, 2024, 8:55 a.m. UTC | #3 
On 06/03/2024 20:56, Igor Pylypiv wrote:
>>> ---
>>>   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++++++
>>>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++++++
>> Is there a reason why you didn't patch:
>> drivers/scsi/hisi_sas/hisi_sas_v1_hw.c ?
>>
> I originally patched hisi_sas_v1_hw.c as well. John Garry pointed out
> that v1 HW doesn't support SATA so I dropped the change.

If you are going to do another spin, then maybe update the commit 
message with this.

Thanks,
John
Niklas Cassel March 7, 2024, 9:59 a.m. UTC | #4 
On Thu, Mar 07, 2024 at 08:55:51AM +0000, John Garry wrote:
> On 06/03/2024 20:56, Igor Pylypiv wrote:
> > > > ---
> > > >   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++++++
> > > >   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++++++
> > > Is there a reason why you didn't patch:
> > > drivers/scsi/hisi_sas/hisi_sas_v1_hw.c ?
> > > 
> > I originally patched hisi_sas_v1_hw.c as well. John Garry pointed out
> > that v1 HW doesn't support SATA so I dropped the change.
> 
> If you are going to do another spin, then maybe update the commit message
> with this.

+1


John, now I'm curious, do you know why hisi_sas_v1_hw.c is implemented
as a libsas driver (instead of a regular SCSI driver), if it doesn't
support SATA?

Was perhaps v2_hw and v3_hw implemented as a libsas driver first
(since they support SATA), and v1_hw support was added later,
so that it could reuse much of the parts of the existing driver?


Kind regards,
Niklas
John Garry March 7, 2024, 11:17 a.m. UTC | #5 
> John, now I'm curious, do you know why hisi_sas_v1_hw.c is implemented
> as a libsas driver (instead of a regular SCSI driver), if it doesn't
> support SATA?

Using libsas is not really dependent on whether the HW supports SATA/STP 
or not. It's a protocol layer thing. Considering the SAS protocol stack, 
HW for drivers using libsas have implemented only the lower protocol 
layers, and SW, i.e. libsas, is required to manage upper layers, like 
Port layer and above.

BTW, IIRC, v1 hw did support SATA, but not STP, i.e. directly attached 
SATA only, but the SATA support was even more broken than v2 hw (which 
is quite broken), so never bothered supporting in SW.

> 
> Was perhaps v2_hw and v3_hw implemented as a libsas driver first
> (since they support SATA), and v1_hw support was added later,
> so that it could reuse much of the parts of the existing driver?

v1 driver support came first.

Thanks,
John
diff mbox series

Patch

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 73b378837da7..b5d379ebe05d 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3544,6 +3544,11 @@  static struct attribute *host_v2_hw_attrs[] = {
 
 ATTRIBUTE_GROUPS(host_v2_hw);
 
+static const struct attribute_group *sdev_groups_v2_hw[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
+
 static void map_queues_v2_hw(struct Scsi_Host *shost)
 {
 	struct hisi_hba *hisi_hba = shost_priv(shost);
@@ -3585,6 +3590,7 @@  static const struct scsi_host_template sht_v2_hw = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.shost_groups		= host_v2_hw_groups,
+	.sdev_groups		= sdev_groups_v2_hw,
 	.host_reset		= hisi_sas_host_reset,
 	.map_queues		= map_queues_v2_hw,
 	.host_tagset		= 1,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index b56fbc61a15a..9b69ea16a1e6 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -2929,6 +2929,11 @@  static struct attribute *host_v3_hw_attrs[] = {
 
 ATTRIBUTE_GROUPS(host_v3_hw);
 
+static const struct attribute_group *sdev_groups_v3_hw[] = {
+	&sas_ata_sdev_attr_group,
+	NULL
+};
+
 #define HISI_SAS_DEBUGFS_REG(x) {#x, x}
 
 struct hisi_sas_debugfs_reg_lu {
@@ -3340,6 +3345,7 @@  static const struct scsi_host_template sht_v3_hw = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.shost_groups		= host_v3_hw_groups,
+	.sdev_groups		= sdev_groups_v3_hw,
 	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,
 	.host_reset             = hisi_sas_host_reset,
 	.host_tagset		= 1,