Re: [PATCH 4/7] pm80xx: Add sysfs attribute to track iop1 count

From: John Garry <john.garry@huawei.com>
To: Viswas G <Viswas.G@microchip.com>, <linux-scsi@vger.kernel.org>
Cc: <Vasanthalakshmi.Tharmarajan@microchip.com>,
	<Ruksar.devadi@microchip.com>, <vishakhavc@google.com>,
	<radha@google.com>, <jinpu.wang@cloud.ionos.com>
Subject: Re: [PATCH 4/7] pm80xx: Add sysfs attribute to track iop1 count
Date: Fri, 5 Mar 2021 08:02:12 +0000	[thread overview]
Message-ID: <1e46d305-a34b-ebda-7bcc-c4311b5405fb@huawei.com> (raw)
In-Reply-To: <20210224155802.13292-5-Viswas.G@microchip.com>

On 24/02/2021 15:57, Viswas G wrote:
> From: Vishakha Channapattan <vishakhavc@google.com>
> 
> A new sysfs variable 'ctl_iop1_count' is being introduced that tells if
> the controller is alive by indicating controller ticks. If on subsequent
> run we see the ticks changing that indicates that controller is not
> dead.
> 

Some comments, if you don't mind:

> Tested: Using 'ctl_iop1_count' sysfs variable we can see ticks
> incrementing
> mvae14:~# cat  /sys/class/scsi_host/host*/ctl_iop1_count
> IOP1TCNT=0x00000069

why does this file not just hold the value, and rather print "IOP1TCNT=" 
as well?

> IOP1TCNT=0x0000006b
> IOP1TCNT=0x0000006d
> IOP1TCNT=0x00000072
> 
> Signed-off-by: Vishakha Channapattan <vishakhavc@google.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
> Signed-off-by: Ruksar Devadi <Ruksar.devadi@microchip.com>
> Signed-off-by: Ashokkumar N <Ashokkumar.N@microchip.com>
> Signed-off-by: Radha Ramachandran <radha@google.com>
> ---
>   drivers/scsi/pm8001/pm8001_ctl.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
> index 8470bce2cee1..9bc9ef446801 100644
> --- a/drivers/scsi/pm8001/pm8001_ctl.c
> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> @@ -967,6 +967,30 @@ static ssize_t ctl_iop0_count_show(struct device *cdev,
>   }
>   static DEVICE_ATTR_RO(ctl_iop0_count);
>   
> +/**
> + * ctl_iop1_count_show - controller iop1 count check
> + * @cdev: pointer to embedded class device
> + * @buf: the buffer returned
> + *
> + * A sysfs 'read-only' shost attribute.
> + */
> +
> +static ssize_t ctl_iop1_count_show(struct device *cdev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	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;
> +	unsigned int iop1cnt = 0;

no need to set an initial value

> +	int c;
> +
> +	pm8001_dbg(pm8001_ha, IOCTL, "%s\n", __func__);

strange that you require a debug message for something so simple

> +	iop1cnt = pm8001_mr32(pm8001_ha->general_stat_tbl_addr, 20);
> +	c = sprintf(buf, "IOP1TCNT=0x%08x\n", iop1cnt); > +	return c;
> +}
> +static DEVICE_ATTR_RO(ctl_iop1_count);

Seems like a lot of duplication in these functions

> +
>   struct device_attribute *pm8001_host_attrs[] = {
>   	&dev_attr_interface_rev,
>   	&dev_attr_controller_fatal_error,
> @@ -993,6 +1017,7 @@ struct device_attribute *pm8001_host_attrs[] = {
>   	&dev_attr_ctl_mpi_state,
>   	&dev_attr_ctl_raae_count,
>   	&dev_attr_ctl_iop0_count,
> +	&dev_attr_ctl_iop1_count,
>   	NULL,
>   };
>   
> 