[PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Asai Thambi S P]

This patch removes entries 'registers' and 'flags' from sysfs. Updated ABI file
to reflect this change.

Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Asai Thambi S P <asamymuthupa@micron.com>
---
 Documentation/ABI/testing/sysfs-block-rssd |   21 ------
 drivers/block/mtip32xx/mtip32xx.c          |   92 +---------------------------
 2 files changed, 1 insertions(+), 112 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-block-rssd b/Documentation/ABI/testing/sysfs-block-rssd
index 679ce35..beef30c 100644
--- a/Documentation/ABI/testing/sysfs-block-rssd
+++ b/Documentation/ABI/testing/sysfs-block-rssd
@@ -1,26 +1,5 @@
-What:           /sys/block/rssd*/registers
-Date:           March 2012
-KernelVersion:  3.3
-Contact:        Asai Thambi S P <asamymuthupa@micron.com>
-Description:    This is a read-only file. Dumps below driver information and
-                hardware registers.
-                    - S ACTive
-                    - Command Issue
-                    - Completed
-                    - PORT IRQ STAT
-                    - HOST IRQ STAT
-                    - Allocated
-                    - Commands in Q
-
 What:           /sys/block/rssd*/status
 Date:           April 2012
 KernelVersion:  3.4
 Contact:        Asai Thambi S P <asamymuthupa@micron.com>
 Description:    This is a read-only file. Indicates the status of the device.
-
-What:           /sys/block/rssd*/flags
-Date:           May 2012
-KernelVersion:  3.5
-Contact:        Asai Thambi S P <asamymuthupa@micron.com>
-Description:    This is a read-only file. Dumps the flags in port and driver
-                data structure
diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 264bc77..b6e95b9 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -2546,7 +2546,7 @@ static struct scatterlist *mtip_hw_get_scatterlist(struct driver_data *dd,
 }
 
 /*
- * Sysfs register/status dump.
+ * Sysfs status dump.
  *
  * @dev  Pointer to the device structure, passed by the kernrel.
  * @attr Pointer to the device_attribute structure passed by the kernel.
@@ -2555,71 +2555,6 @@ static struct scatterlist *mtip_hw_get_scatterlist(struct driver_data *dd,
  * return value
  *	The size, in bytes, of the data copied into buf.
  */
-static ssize_t mtip_hw_show_registers(struct device *dev,
-				struct device_attribute *attr,
-				char *buf)
-{
-	u32 group_allocated;
-	struct driver_data *dd = dev_to_disk(dev)->private_data;
-	int size = 0;
-	int n;
-
-	size += sprintf(&buf[size], "Hardware\n--------\n");
-	size += sprintf(&buf[size], "S ACTive      : [ 0x");
-
-	for (n = dd->slot_groups-1; n >= 0; n--)
-		size += sprintf(&buf[size], "%08X ",
-					 readl(dd->port->s_active[n]));
-
-	size += sprintf(&buf[size], "]\n");
-	size += sprintf(&buf[size], "Command Issue : [ 0x");
-
-	for (n = dd->slot_groups-1; n >= 0; n--)
-		size += sprintf(&buf[size], "%08X ",
-					readl(dd->port->cmd_issue[n]));
-
-	size += sprintf(&buf[size], "]\n");
-	size += sprintf(&buf[size], "Completed     : [ 0x");
-
-	for (n = dd->slot_groups-1; n >= 0; n--)
-		size += sprintf(&buf[size], "%08X ",
-				readl(dd->port->completed[n]));
-
-	size += sprintf(&buf[size], "]\n");
-	size += sprintf(&buf[size], "PORT IRQ STAT : [ 0x%08X ]\n",
-				readl(dd->port->mmio + PORT_IRQ_STAT));
-	size += sprintf(&buf[size], "HOST IRQ STAT : [ 0x%08X ]\n",
-				readl(dd->mmio + HOST_IRQ_STAT));
-	size += sprintf(&buf[size], "\n");
-
-	size += sprintf(&buf[size], "Local\n-----\n");
-	size += sprintf(&buf[size], "Allocated    : [ 0x");
-
-	for (n = dd->slot_groups-1; n >= 0; n--) {
-		if (sizeof(long) > sizeof(u32))
-			group_allocated =
-				dd->port->allocated[n/2] >> (32*(n&1));
-		else
-			group_allocated = dd->port->allocated[n];
-		size += sprintf(&buf[size], "%08X ", group_allocated);
-	}
-	size += sprintf(&buf[size], "]\n");
-
-	size += sprintf(&buf[size], "Commands in Q: [ 0x");
-
-	for (n = dd->slot_groups-1; n >= 0; n--) {
-		if (sizeof(long) > sizeof(u32))
-			group_allocated =
-				dd->port->cmds_to_issue[n/2] >> (32*(n&1));
-		else
-			group_allocated = dd->port->cmds_to_issue[n];
-		size += sprintf(&buf[size], "%08X ", group_allocated);
-	}
-	size += sprintf(&buf[size], "]\n");
-
-	return size;
-}
-
 static ssize_t mtip_hw_show_status(struct device *dev,
 				struct device_attribute *attr,
 				char *buf)
@@ -2637,24 +2572,7 @@ static ssize_t mtip_hw_show_status(struct device *dev,
 	return size;
 }
 
-static ssize_t mtip_hw_show_flags(struct device *dev,
-				struct device_attribute *attr,
-				char *buf)
-{
-	struct driver_data *dd = dev_to_disk(dev)->private_data;
-	int size = 0;
-
-	size += sprintf(&buf[size], "Flag in port struct : [ %08lX ]\n",
-							dd->port->flags);
-	size += sprintf(&buf[size], "Flag in dd struct   : [ %08lX ]\n",
-							dd->dd_flag);
-
-	return size;
-}
-
-static DEVICE_ATTR(registers, S_IRUGO, mtip_hw_show_registers, NULL);
 static DEVICE_ATTR(status, S_IRUGO, mtip_hw_show_status, NULL);
-static DEVICE_ATTR(flags, S_IRUGO, mtip_hw_show_flags, NULL);
 
 /*
  * Create the sysfs related attributes.
@@ -2671,15 +2589,9 @@ static int mtip_hw_sysfs_init(struct driver_data *dd, struct kobject *kobj)
 	if (!kobj || !dd)
 		return -EINVAL;
 
-	if (sysfs_create_file(kobj, &dev_attr_registers.attr))
-		dev_warn(&dd->pdev->dev,
-			"Error creating 'registers' sysfs entry\n");
 	if (sysfs_create_file(kobj, &dev_attr_status.attr))
 		dev_warn(&dd->pdev->dev,
 			"Error creating 'status' sysfs entry\n");
-	if (sysfs_create_file(kobj, &dev_attr_flags.attr))
-		dev_warn(&dd->pdev->dev,
-			"Error creating 'flags' sysfs entry\n");
 	return 0;
 }
 
@@ -2698,9 +2610,7 @@ static int mtip_hw_sysfs_exit(struct driver_data *dd, struct kobject *kobj)
 	if (!kobj || !dd)
 		return -EINVAL;
 
-	sysfs_remove_file(kobj, &dev_attr_registers.attr);
 	sysfs_remove_file(kobj, &dev_attr_status.attr);
-	sysfs_remove_file(kobj, &dev_attr_flags.attr);
 
 	return 0;
 }
-- 
1.7.1

Re: [PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Jens Axboe]

On 06/04/2012 09:43 PM, Asai Thambi S P wrote:
> 
> This patch removes entries 'registers' and 'flags' from sysfs. Updated
> ABI file to reflect this change.

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Greg KH]

On Mon, Jun 04, 2012 at 12:43:03PM -0700, Asai Thambi S P wrote:
> 
> This patch removes entries 'registers' and 'flags' from sysfs. Updated ABI file
> to reflect this change.
> 
> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Asai Thambi S P <asamymuthupa@micron.com>

Much nicer, thanks for doing this:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

But, one question on a different sysfs file:

>  What:           /sys/block/rssd*/status
>  Date:           April 2012
>  KernelVersion:  3.4
>  Contact:        Asai Thambi S P <asamymuthupa@micron.com>
>  Description:    This is a read-only file. Indicates the status of the device.

What "status" is this showing?  Why is this a sysfs file?  Who
needs/wants it?

Also, if you really want to properly create sysfs files, use the default
attributes for the driver.  As it is, you are creating them after
userspace is notified about the device showing up, which races with the
creation of your additional file(s).  Use the properly driver core field
to have the core create, and remove them, automatically, which saves you
code, and fixes bugs you didn't realize you had :)

thanks,

greg k-h

Re: [PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Asai Thambi S P]

On 6/5/2012 2:33 AM, Greg KH wrote:

> On Mon, Jun 04, 2012 at 12:43:03PM -0700, Asai Thambi S P wrote:
>>
>> This patch removes entries 'registers' and 'flags' from sysfs. Updated ABI file
>> to reflect this change.
>>
>> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Asai Thambi S P <asamymuthupa@micron.com>
> 
> Much nicer, thanks for doing this:
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> But, one question on a different sysfs file:
> 
>>  What:           /sys/block/rssd*/status
>>  Date:           April 2012
>>  KernelVersion:  3.4
>>  Contact:        Asai Thambi S P <asamymuthupa@micron.com>
>>  Description:    This is a read-only file. Indicates the status of the device.
> 
> What "status" is this showing?  Why is this a sysfs file?  Who
> needs/wants it?


This shows the device status - online, write_protect or thermal_shutdown. This
would be used by management application.

> 
> Also, if you really want to properly create sysfs files, use the default
> attributes for the driver.  As it is, you are creating them after
> userspace is notified about the device showing up, which races with the
> creation of your additional file(s).  Use the properly driver core field
> to have the core create, and remove them, automatically, which saves you
> code, and fixes bugs you didn't realize you had :)
 

I have not thought about this. Thanks for the insight. I will look into it.

--
Regards,
Asai Thambi

Re: [PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Greg KH]

On Tue, Jun 05, 2012 at 11:18:15AM -0700, Asai Thambi S P wrote:
> On 6/5/2012 2:33 AM, Greg KH wrote:
> 
> > On Mon, Jun 04, 2012 at 12:43:03PM -0700, Asai Thambi S P wrote:
> >>
> >> This patch removes entries 'registers' and 'flags' from sysfs. Updated ABI file
> >> to reflect this change.
> >>
> >> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Signed-off-by: Asai Thambi S P <asamymuthupa@micron.com>
> > 
> > Much nicer, thanks for doing this:
> > 
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > But, one question on a different sysfs file:
> > 
> >>  What:           /sys/block/rssd*/status
> >>  Date:           April 2012
> >>  KernelVersion:  3.4
> >>  Contact:        Asai Thambi S P <asamymuthupa@micron.com>
> >>  Description:    This is a read-only file. Indicates the status of the device.
> > 
> > What "status" is this showing?  Why is this a sysfs file?  Who
> > needs/wants it?
> 
> 
> This shows the device status - online, write_protect or thermal_shutdown. This
> would be used by management application.

Is it used by a management application?  Shouldn't such a tool use the
"standard" block device status files instead?  I thought we exported
that information already in the /sys/block/* files.

thanks,

greg k-h

Re: [PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Asai Thambi S P]

On 6/5/2012 1:25 PM, Greg KH wrote:

> On Tue, Jun 05, 2012 at 11:18:15AM -0700, Asai Thambi S P wrote:
>> On 6/5/2012 2:33 AM, Greg KH wrote:
>>
>>> On Mon, Jun 04, 2012 at 12:43:03PM -0700, Asai Thambi S P wrote:
>>>>
>>>> This patch removes entries 'registers' and 'flags' from sysfs. Updated ABI file
>>>> to reflect this change.
>>>>
>>>> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Signed-off-by: Asai Thambi S P <asamymuthupa@micron.com>
>>>
>>> Much nicer, thanks for doing this:
>>>
>>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>
>>> But, one question on a different sysfs file:
>>>
>>>>  What:           /sys/block/rssd*/status
>>>>  Date:           April 2012
>>>>  KernelVersion:  3.4
>>>>  Contact:        Asai Thambi S P <asamymuthupa@micron.com>
>>>>  Description:    This is a read-only file. Indicates the status of the device.
>>>
>>> What "status" is this showing?  Why is this a sysfs file?  Who
>>> needs/wants it?
>>
>>
>> This shows the device status - online, write_protect or thermal_shutdown. This
>> would be used by management application.
> 
> Is it used by a management application?  Shouldn't such a tool use the
> "standard" block device status files instead?  I thought we exported
> that information already in the /sys/block/* files.
> 


device status is not exported by standard block interface.

--
Regards,
Asai Thambi

Re: [PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Greg KH]

On Wed, Jun 06, 2012 at 11:04:17AM -0700, Asai Thambi S P wrote:
> On 6/5/2012 1:25 PM, Greg KH wrote:
> 
> > On Tue, Jun 05, 2012 at 11:18:15AM -0700, Asai Thambi S P wrote:
> >> On 6/5/2012 2:33 AM, Greg KH wrote:
> >>
> >>> On Mon, Jun 04, 2012 at 12:43:03PM -0700, Asai Thambi S P wrote:
> >>>>
> >>>> This patch removes entries 'registers' and 'flags' from sysfs. Updated ABI file
> >>>> to reflect this change.
> >>>>
> >>>> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>>> Signed-off-by: Asai Thambi S P <asamymuthupa@micron.com>
> >>>
> >>> Much nicer, thanks for doing this:
> >>>
> >>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>>
> >>> But, one question on a different sysfs file:
> >>>
> >>>>  What:           /sys/block/rssd*/status
> >>>>  Date:           April 2012
> >>>>  KernelVersion:  3.4
> >>>>  Contact:        Asai Thambi S P <asamymuthupa@micron.com>
> >>>>  Description:    This is a read-only file. Indicates the status of the device.
> >>>
> >>> What "status" is this showing?  Why is this a sysfs file?  Who
> >>> needs/wants it?
> >>
> >>
> >> This shows the device status - online, write_protect or thermal_shutdown. This
> >> would be used by management application.
> > 
> > Is it used by a management application?  Shouldn't such a tool use the
> > "standard" block device status files instead?  I thought we exported
> > that information already in the /sys/block/* files.
> > 
> 
> 
> device status is not exported by standard block interface.

Ok, but don't you think it should be?  Why make this unique to only your
driver?

greg k-h

Re: [PATCH 1/2] mtip32xx: Remove 'registers' and 'flags' from sysfs

[Asai Thambi S P]

On 6/6/2012 2:25 PM, Greg KH wrote:

> On Wed, Jun 06, 2012 at 11:04:17AM -0700, Asai Thambi S P wrote:
>> On 6/5/2012 1:25 PM, Greg KH wrote:
>>
>>> On Tue, Jun 05, 2012 at 11:18:15AM -0700, Asai Thambi S P wrote:
>>>> On 6/5/2012 2:33 AM, Greg KH wrote:
>>>>
>>>>> On Mon, Jun 04, 2012 at 12:43:03PM -0700, Asai Thambi S P wrote:
>>>>>>
>>>>>> This patch removes entries 'registers' and 'flags' from sysfs. Updated ABI file
>>>>>> to reflect this change.
>>>>>>
>>>>>> Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>> Signed-off-by: Asai Thambi S P <asamymuthupa@micron.com>
>>>>>
>>>>> Much nicer, thanks for doing this:
>>>>>
>>>>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>
>>>>> But, one question on a different sysfs file:
>>>>>
>>>>>>  What:           /sys/block/rssd*/status
>>>>>>  Date:           April 2012
>>>>>>  KernelVersion:  3.4
>>>>>>  Contact:        Asai Thambi S P <asamymuthupa@micron.com>
>>>>>>  Description:    This is a read-only file. Indicates the status of the device.
>>>>>
>>>>> What "status" is this showing?  Why is this a sysfs file?  Who
>>>>> needs/wants it?
>>>>
>>>>
>>>> This shows the device status - online, write_protect or thermal_shutdown. This
>>>> would be used by management application.
>>>
>>> Is it used by a management application?  Shouldn't such a tool use the
>>> "standard" block device status files instead?  I thought we exported
>>> that information already in the /sys/block/* files.
>>>
>>
>>
>> device status is not exported by standard block interface.
> 
> Ok, but don't you think it should be?  Why make this unique to only your
> driver?
> 


It is unique to our driver because it exports information that is unique. In
addition to write protect and thermal status, in future it would export the
status of what we call “ftl rebuild”, which is a rebuilding of the logical to
physical mappings internal to the flash controller. If and when other devices
need to export device status, it could be moved to standard block interface.

--
Regards,
Asai Thambi