diff mbox series

[06/12] pm80xx : sysfs attribute for number of phys.

Message ID 20191224044143.8178-7-deepak.ukey@microchip.com (mailing list archive)
State Changes Requested
Headers show
Series pm80xx : Updates for the driver version 0.1.39. | expand

Commit Message

Deepak Ukey Dec. 24, 2019, 4:41 a.m. UTC 
From: Viswas G <Viswas.G@microchip.com>

Added sysfs attribute to show number of phys.

Signed-off-by: Deepak Ukey <deepak.ukey@microchip.com>
Signed-off-by: Viswas G <Viswas.G@microchip.com>
Signed-off-by: Vishakha Channapattan <vishakhavc@google.com>
Signed-off-by: Bhavesh Jashnani <bjashnani@google.com>
Signed-off-by: Radha Ramachandran <radha@google.com>
Signed-off-by: Akshat Jain <akshatzen@google.com>
Signed-off-by: Yu Zheng <yuuzheng@google.com>
---
 drivers/scsi/pm8001/pm8001_ctl.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Jinpu Wang Jan. 2, 2020, 12:07 p.m. UTC | #1 
On Tue, Dec 24, 2019 at 5:41 AM Deepak Ukey <deepak.ukey@microchip.com> wrote:
>
> From: Viswas G <Viswas.G@microchip.com>
>
> Added sysfs attribute to show number of phys.
>
> Signed-off-by: Deepak Ukey <deepak.ukey@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> Signed-off-by: Vishakha Channapattan <vishakhavc@google.com>
> Signed-off-by: Bhavesh Jashnani <bjashnani@google.com>
> Signed-off-by: Radha Ramachandran <radha@google.com>
> Signed-off-by: Akshat Jain <akshatzen@google.com>
> Signed-off-by: Yu Zheng <yuuzheng@google.com>
> ---
>  drivers/scsi/pm8001/pm8001_ctl.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
> index 69458b318a20..704c0daa7937 100644
> --- a/drivers/scsi/pm8001/pm8001_ctl.c
> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> @@ -89,6 +89,25 @@ static ssize_t controller_fatal_error_show(struct device *cdev,
>  }
>  static DEVICE_ATTR_RO(controller_fatal_error);
>
> +/**
> + * pm8001_ctl_num_phys_show - Number of phys
> + * @cdev:pointer to embedded class device
> + * @buf:the buffer returned
> + * A sysfs 'read-only' shost attribute.
> + */
> +static ssize_t num_phys_show(struct device *cdev,
> +               struct device_attribute *attr, char *buf)
please use pm8001_ctl_num_phys_show as function name, so it follows
same conversion as other functions.
Better also rename controller_fatal_error too.

Thanks
> +{
> +       int ret;
> +       struct Scsi_Host *shost = class_to_shost(cdev);
> +       struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> +       struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
> +
> +       ret = sprintf(buf, "%d", pm8001_ha->chip->n_phy);
> +       return ret;
> +}
> +static DEVICE_ATTR_RO(num_phys);
> +
>  /**
>   * pm8001_ctl_fw_version_show - firmware version
>   * @cdev: pointer to embedded class device
> @@ -825,6 +844,7 @@ static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUSR|S_IWGRP,
>  struct device_attribute *pm8001_host_attrs[] = {
>         &dev_attr_interface_rev,
>         &dev_attr_controller_fatal_error,
> +       &dev_attr_num_phys,
>         &dev_attr_fw_version,
>         &dev_attr_update_fw,
>         &dev_attr_aap_log,
> --
> 2.16.3
>
John Garry Jan. 2, 2020, 12:33 p.m. UTC | #2 
On 02/01/2020 12:07, Jinpu Wang wrote:
> On Tue, Dec 24, 2019 at 5:41 AM Deepak Ukey <deepak.ukey@microchip.com> wrote:
>>
>> From: Viswas G <Viswas.G@microchip.com>
>>
>> Added sysfs attribute to show number of phys.
>>
>> Signed-off-by: Deepak Ukey <deepak.ukey@microchip.com>
>> Signed-off-by: Viswas G <Viswas.G@microchip.com>
>> Signed-off-by: Vishakha Channapattan <vishakhavc@google.com>
>> Signed-off-by: Bhavesh Jashnani <bjashnani@google.com>
>> Signed-off-by: Radha Ramachandran <radha@google.com>
>> Signed-off-by: Akshat Jain <akshatzen@google.com>
>> Signed-off-by: Yu Zheng <yuuzheng@google.com>
>> ---
>>   drivers/scsi/pm8001/pm8001_ctl.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
>> index 69458b318a20..704c0daa7937 100644
>> --- a/drivers/scsi/pm8001/pm8001_ctl.c
>> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
>> @@ -89,6 +89,25 @@ static ssize_t controller_fatal_error_show(struct device *cdev,
>>   }
>>   static DEVICE_ATTR_RO(controller_fatal_error);
>>
>> +/**
>> + * pm8001_ctl_num_phys_show - Number of phys
>> + * @cdev:pointer to embedded class device
>> + * @buf:the buffer returned
>> + * A sysfs 'read-only' shost attribute.
>> + */
>> +static ssize_t num_phys_show(struct device *cdev,
>> +               struct device_attribute *attr, char *buf)
> please use pm8001_ctl_num_phys_show as function name, so it follows
> same conversion as other functions.
> Better also rename controller_fatal_error too.

If you don't mind me saying, this info is already available in sysfs for 
any libsas or even SAS host (using scsi_transport_sas.c), like this:

john@ubuntu:/sys/class/sas_phy$ ls
phy-0:0     phy-0:0:12  phy-0:0:17  phy-0:0:21  phy-0:0:4  phy-0:0:9 
phy-0:5
phy-0:0:0   phy-0:0:13  phy-0:0:18  phy-0:0:22  phy-0:0:5  phy-0:1 
phy-0:6
phy-0:0:1   phy-0:0:14  phy-0:0:19  phy-0:0:23  phy-0:0:6  phy-0:2 
phy-0:7
phy-0:0:10  phy-0:0:15  phy-0:0:2   phy-0:0:24  phy-0:0:7  phy-0:3
phy-0:0:11  phy-0:0:16  phy-0:0:20  phy-0:0:3   phy-0:0:8  phy-0:4


Any phy-X:Y is a root phy, and X denotes the host index and Y is the phy 
index.

> 
> Thanks
>> +{
>> +       int ret;
>> +       struct Scsi_Host *shost = class_to_shost(cdev);
>> +       struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
>> +       struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
>> +
>> +       ret = sprintf(buf, "%d", pm8001_ha->chip->n_phy);
>> +       return ret;
>> +}
>> +static DEVICE_ATTR_RO(num_phys);
>> +
>>   /**
>>    * pm8001_ctl_fw_version_show - firmware version
>>    * @cdev: pointer to embedded class device
>> @@ -825,6 +844,7 @@ static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUSR|S_IWGRP,
>>   struct device_attribute *pm8001_host_attrs[] = {
>>          &dev_attr_interface_rev,
>>          &dev_attr_controller_fatal_error,
>> +       &dev_attr_num_phys,
>>          &dev_attr_fw_version,
>>          &dev_attr_update_fw,
>>          &dev_attr_aap_log,
>> --
>> 2.16.3
>>
> .
>
Viswas G Jan. 2, 2020, 4:38 p.m. UTC | #3 
Thanks John Garry. The intention here is to get the total number of phys for the controller through sysfs and it is used by the management utility as well. 

Regards,
Viswas G

-----Original Message-----
From: John Garry <john.garry@huawei.com> 
Sent: Thursday, January 2, 2020 6:03 PM
To: Jinpu Wang <jinpu.wang@cloud.ionos.com>; Deepak Ukey - I31172 <Deepak.Ukey@microchip.com>
Cc: Linux SCSI Mailinglist <linux-scsi@vger.kernel.org>; Vasanthalakshmi Tharmarajan - I30664 <Vasanthalakshmi.Tharmarajan@microchip.com>; Viswas G - I30667 <Viswas.G@microchip.com>; Jack Wang <jinpu.wang@profitbricks.com>; Martin K. Petersen <martin.petersen@oracle.com>; dpf@google.com; yuuzheng@google.com; Vikram Auradkar <auradkar@google.com>; vishakhavc@google.com; bjashnani@google.com; Radha Ramachandran <radha@google.com>; akshatzen@google.com
Subject: Re: [PATCH 06/12] pm80xx : sysfs attribute for number of phys.

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On 02/01/2020 12:07, Jinpu Wang wrote:
> On Tue, Dec 24, 2019 at 5:41 AM Deepak Ukey <mailto:deepak.ukey@microchip.com> wrote:
>>
>> From: Viswas G <mailto:Viswas.G@microchip.com>
>>
>> Added sysfs attribute to show number of phys.
>>
>> Signed-off-by: Deepak Ukey <mailto:deepak.ukey@microchip.com>
>> Signed-off-by: Viswas G <mailto:Viswas.G@microchip.com>
>> Signed-off-by: Vishakha Channapattan <mailto:vishakhavc@google.com>
>> Signed-off-by: Bhavesh Jashnani <mailto:bjashnani@google.com>
>> Signed-off-by: Radha Ramachandran <mailto:radha@google.com>
>> Signed-off-by: Akshat Jain <mailto:akshatzen@google.com>
>> Signed-off-by: Yu Zheng <mailto:yuuzheng@google.com>
>> ---
>>   drivers/scsi/pm8001/pm8001_ctl.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c 
>> b/drivers/scsi/pm8001/pm8001_ctl.c
>> index 69458b318a20..704c0daa7937 100644
>> --- a/drivers/scsi/pm8001/pm8001_ctl.c
>> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
>> @@ -89,6 +89,25 @@ static ssize_t controller_fatal_error_show(struct device *cdev,
>>   }
>>   static DEVICE_ATTR_RO(controller_fatal_error);
>>
>> +/**
>> + * pm8001_ctl_num_phys_show - Number of phys
>> + * @cdev:pointer to embedded class device
>> + * @buf:the buffer returned
>> + * A sysfs 'read-only' shost attribute.
>> + */
>> +static ssize_t num_phys_show(struct device *cdev,
>> +               struct device_attribute *attr, char *buf)
> please use pm8001_ctl_num_phys_show as function name, so it follows 
> same conversion as other functions.
> Better also rename controller_fatal_error too.

If you don't mind me saying, this info is already available in sysfs for any libsas or even SAS host (using scsi_transport_sas.c), like this:

john@ubuntu:/sys/class/sas_phy$ ls
phy-0:0     phy-0:0:12  phy-0:0:17  phy-0:0:21  phy-0:0:4  phy-0:0:9
phy-0:5
phy-0:0:0   phy-0:0:13  phy-0:0:18  phy-0:0:22  phy-0:0:5  phy-0:1
phy-0:6
phy-0:0:1   phy-0:0:14  phy-0:0:19  phy-0:0:23  phy-0:0:6  phy-0:2
phy-0:7
phy-0:0:10  phy-0:0:15  phy-0:0:2   phy-0:0:24  phy-0:0:7  phy-0:3
phy-0:0:11  phy-0:0:16  phy-0:0:20  phy-0:0:3   phy-0:0:8  phy-0:4


Any phy-X:Y is a root phy, and X denotes the host index and Y is the phy index.

>
> Thanks
>> +{
>> +       int ret;
>> +       struct Scsi_Host *shost = class_to_shost(cdev);
>> +       struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
>> +       struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
>> +
>> +       ret = sprintf(buf, "%d", pm8001_ha->chip->n_phy);
>> +       return ret;
>> +}
>> +static DEVICE_ATTR_RO(num_phys);
>> +
>>   /**
>>    * pm8001_ctl_fw_version_show - firmware version
>>    * @cdev: pointer to embedded class device @@ -825,6 +844,7 @@ 
>> static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUSR|S_IWGRP,
>>   struct device_attribute *pm8001_host_attrs[] = {
>>          &dev_attr_interface_rev,
>>          &dev_attr_controller_fatal_error,
>> +       &dev_attr_num_phys,
>>          &dev_attr_fw_version,
>>          &dev_attr_update_fw,
>>          &dev_attr_aap_log,
>> --
>> 2.16.3
>>
> .
>
Deepak Ukey Jan. 13, 2020, 9:26 a.m. UTC | #4 
Hi Jinpu,

please use pm8001_ctl_num_phys_show as function name, so it follows 
> same conversion as other functions.
> Better also rename controller_fatal_error too.

If I tried to keep the function name as pm8001_ctl_num_phys_show as other function follows then checkpatch.pl gives the below warning. 

------
WARNING: Consider renaming function(s) 'pm8001_ctl_num_phys_show' to 'num_phys_show'
#37: FILE: drivers/scsi/pm8001/pm8001_ctl.c:108:
+}

total: 0 errors, 1 warnings, 32 lines checked
-------

That’s the only reason  I have changes the function name. For making the other function with same naming convention, I will change the function name of all other function like num_phys_show' and we will submit this patch in  our upcoming patch set.

Regards,
Deepak
  
-----Original Message-----
From: Viswas G - I30667 
Sent: Thursday, January 2, 2020 10:09 PM
To: John Garry <john.garry@huawei.com>; Jinpu Wang <jinpu.wang@cloud.ionos.com>; Deepak Ukey - I31172 <Deepak.Ukey@microchip.com>
Cc: Linux SCSI Mailinglist <linux-scsi@vger.kernel.org>; Vasanthalakshmi Tharmarajan - I30664 <Vasanthalakshmi.Tharmarajan@microchip.com>; Jack Wang <jinpu.wang@profitbricks.com>; Martin K. Petersen <martin.petersen@oracle.com>; dpf@google.com; yuuzheng@google.com; Vikram Auradkar <auradkar@google.com>; vishakhavc@google.com; bjashnani@google.com; Radha Ramachandran <radha@google.com>; akshatzen@google.com
Subject: RE: [PATCH 06/12] pm80xx : sysfs attribute for number of phys.

Thanks John Garry. The intention here is to get the total number of phys for the controller through sysfs and it is used by the management utility as well. 

Regards,
Viswas G

-----Original Message-----
From: John Garry <john.garry@huawei.com>
Sent: Thursday, January 2, 2020 6:03 PM
To: Jinpu Wang <jinpu.wang@cloud.ionos.com>; Deepak Ukey - I31172 <Deepak.Ukey@microchip.com>
Cc: Linux SCSI Mailinglist <linux-scsi@vger.kernel.org>; Vasanthalakshmi Tharmarajan - I30664 <Vasanthalakshmi.Tharmarajan@microchip.com>; Viswas G - I30667 <Viswas.G@microchip.com>; Jack Wang <jinpu.wang@profitbricks.com>; Martin K. Petersen <martin.petersen@oracle.com>; dpf@google.com; yuuzheng@google.com; Vikram Auradkar <auradkar@google.com>; vishakhavc@google.com; bjashnani@google.com; Radha Ramachandran <radha@google.com>; akshatzen@google.com
Subject: Re: [PATCH 06/12] pm80xx : sysfs attribute for number of phys.

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On 02/01/2020 12:07, Jinpu Wang wrote:
> On Tue, Dec 24, 2019 at 5:41 AM Deepak Ukey <mailto:deepak.ukey@microchip.com> wrote:
>>
>> From: Viswas G <mailto:Viswas.G@microchip.com>
>>
>> Added sysfs attribute to show number of phys.
>>
>> Signed-off-by: Deepak Ukey <mailto:deepak.ukey@microchip.com>
>> Signed-off-by: Viswas G <mailto:Viswas.G@microchip.com>
>> Signed-off-by: Vishakha Channapattan <mailto:vishakhavc@google.com>
>> Signed-off-by: Bhavesh Jashnani <mailto:bjashnani@google.com>
>> Signed-off-by: Radha Ramachandran <mailto:radha@google.com>
>> Signed-off-by: Akshat Jain <mailto:akshatzen@google.com>
>> Signed-off-by: Yu Zheng <mailto:yuuzheng@google.com>
>> ---
>>   drivers/scsi/pm8001/pm8001_ctl.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c
>> b/drivers/scsi/pm8001/pm8001_ctl.c
>> index 69458b318a20..704c0daa7937 100644
>> --- a/drivers/scsi/pm8001/pm8001_ctl.c
>> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
>> @@ -89,6 +89,25 @@ static ssize_t controller_fatal_error_show(struct device *cdev,
>>   }
>>   static DEVICE_ATTR_RO(controller_fatal_error);
>>
>> +/**
>> + * pm8001_ctl_num_phys_show - Number of phys
>> + * @cdev:pointer to embedded class device
>> + * @buf:the buffer returned
>> + * A sysfs 'read-only' shost attribute.
>> + */
>> +static ssize_t num_phys_show(struct device *cdev,
>> +               struct device_attribute *attr, char *buf)
> please use pm8001_ctl_num_phys_show as function name, so it follows 
> same conversion as other functions.
> Better also rename controller_fatal_error too.

If you don't mind me saying, this info is already available in sysfs for any libsas or even SAS host (using scsi_transport_sas.c), like this:

john@ubuntu:/sys/class/sas_phy$ ls
phy-0:0     phy-0:0:12  phy-0:0:17  phy-0:0:21  phy-0:0:4  phy-0:0:9
phy-0:5
phy-0:0:0   phy-0:0:13  phy-0:0:18  phy-0:0:22  phy-0:0:5  phy-0:1
phy-0:6
phy-0:0:1   phy-0:0:14  phy-0:0:19  phy-0:0:23  phy-0:0:6  phy-0:2
phy-0:7
phy-0:0:10  phy-0:0:15  phy-0:0:2   phy-0:0:24  phy-0:0:7  phy-0:3
phy-0:0:11  phy-0:0:16  phy-0:0:20  phy-0:0:3   phy-0:0:8  phy-0:4


Any phy-X:Y is a root phy, and X denotes the host index and Y is the phy index.

>
> Thanks
>> +{
>> +       int ret;
>> +       struct Scsi_Host *shost = class_to_shost(cdev);
>> +       struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
>> +       struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
>> +
>> +       ret = sprintf(buf, "%d", pm8001_ha->chip->n_phy);
>> +       return ret;
>> +}
>> +static DEVICE_ATTR_RO(num_phys);
>> +
>>   /**
>>    * pm8001_ctl_fw_version_show - firmware version
>>    * @cdev: pointer to embedded class device @@ -825,6 +844,7 @@ 
>> static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUSR|S_IWGRP,
>>   struct device_attribute *pm8001_host_attrs[] = {
>>          &dev_attr_interface_rev,
>>          &dev_attr_controller_fatal_error,
>> +       &dev_attr_num_phys,
>>          &dev_attr_fw_version,
>>          &dev_attr_update_fw,
>>          &dev_attr_aap_log,
>> --
>> 2.16.3
>>
> .
>
Jinpu Wang Jan. 13, 2020, 9:39 a.m. UTC | #5 
On Mon, Jan 13, 2020 at 10:26 AM <Deepak.Ukey@microchip.com> wrote:
>
> Hi Jinpu,
>
> please use pm8001_ctl_num_phys_show as function name, so it follows
> > same conversion as other functions.
> > Better also rename controller_fatal_error too.
>
> If I tried to keep the function name as pm8001_ctl_num_phys_show as other function follows then checkpatch.pl gives the below warning.
>
> ------
> WARNING: Consider renaming function(s) 'pm8001_ctl_num_phys_show' to 'num_phys_show'
> #37: FILE: drivers/scsi/pm8001/pm8001_ctl.c:108:
> +}
>
> total: 0 errors, 1 warnings, 32 lines checked
> -------
>
> That’s the only reason  I have changes the function name. For making the other function with same naming convention, I will change the function name of all other function like num_phys_show' and we will submit this patch in  our upcoming patch set.
>
> Regards,
> Deepak


Thanks for checking, then we can do it later.
PS: please avoid top-posting.
diff mbox series

Patch

diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
index 69458b318a20..704c0daa7937 100644
--- a/drivers/scsi/pm8001/pm8001_ctl.c
+++ b/drivers/scsi/pm8001/pm8001_ctl.c
@@ -89,6 +89,25 @@  static ssize_t controller_fatal_error_show(struct device *cdev,
 }
 static DEVICE_ATTR_RO(controller_fatal_error);
 
+/**
+ * pm8001_ctl_num_phys_show - Number of phys
+ * @cdev:pointer to embedded class device
+ * @buf:the buffer returned
+ * A sysfs 'read-only' shost attribute.
+ */
+static ssize_t num_phys_show(struct device *cdev,
+		struct device_attribute *attr, char *buf)
+{
+	int ret;
+	struct Scsi_Host *shost = class_to_shost(cdev);
+	struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
+	struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
+
+	ret = sprintf(buf, "%d", pm8001_ha->chip->n_phy);
+	return ret;
+}
+static DEVICE_ATTR_RO(num_phys);
+
 /**
  * pm8001_ctl_fw_version_show - firmware version
  * @cdev: pointer to embedded class device
@@ -825,6 +844,7 @@  static DEVICE_ATTR(update_fw, S_IRUGO|S_IWUSR|S_IWGRP,
 struct device_attribute *pm8001_host_attrs[] = {
 	&dev_attr_interface_rev,
 	&dev_attr_controller_fatal_error,
+	&dev_attr_num_phys,
 	&dev_attr_fw_version,
 	&dev_attr_update_fw,
 	&dev_attr_aap_log,