[PATCH v2 0/2] Enclosure interface cleanups

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2 0/2] Enclosure interface cleanups
@ 2023-05-24 11:12 Mariusz Tkaczyk
  2023-05-24 11:12 ` [PATCH v2 1/2] misc: enclosure: remove get_active callback Mariusz Tkaczyk
  2023-05-24 11:12 ` [PATCH v2 2/2] misc: enclosure: update sysfs api Mariusz Tkaczyk
  0 siblings, 2 replies; 5+ messages in thread
From: Mariusz Tkaczyk @ 2023-05-24 11:12 UTC (permalink / raw)
  To: arnd; +Cc: andriy.shevchenko, linux-kernel, dan.j.williams

Hi Arnd,
While looking into adding Native PCIE Enclosure Management (NPEM) support
behind the existing enclosure ABI, I noticed a few opportunities for
cleanups.

Changes since v1[1] + internal review:
- Keep reporting the cached version of ecomp->active (Dan).
- Preserve cached values in ecomp, drop patch 2 (Dan).
- Add comment documenting cached ecomp->active (Dan).
- Move ATTRS macros close to appropriate _show and _store functions (Andy).

[1] https://lore.kernel.org/linux-pci/20221117163407.28472-1-mariusz.tkaczyk@linux.intel.com/

Mariusz Tkaczyk (2):
  misc: enclosure: remove get_active callback
  misc: enclosure: update sysfs api

 drivers/misc/enclosure.c  | 84 ++++++++++++++++++---------------------
 include/linux/enclosure.h |  2 -
 2 files changed, 39 insertions(+), 47 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] misc: enclosure: remove get_active callback
  2023-05-24 11:12 [PATCH v2 0/2] Enclosure interface cleanups Mariusz Tkaczyk
@ 2023-05-24 11:12 ` Mariusz Tkaczyk
  2023-05-25  9:38   ` Christoph Hellwig
  2023-05-24 11:12 ` [PATCH v2 2/2] misc: enclosure: update sysfs api Mariusz Tkaczyk
  1 sibling, 1 reply; 5+ messages in thread
From: Mariusz Tkaczyk @ 2023-05-24 11:12 UTC (permalink / raw)
  To: arnd; +Cc: andriy.shevchenko, linux-kernel, dan.j.williams

The callback is not used, remove it. Leave possibility to read cached
ecomp->active value.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/misc/enclosure.c  | 4 +---
 include/linux/enclosure.h | 2 --
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 1b010d9267c9..59704fdb962a 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -534,11 +534,9 @@ static ssize_t set_component_status(struct device *cdev,
 static ssize_t get_component_active(struct device *cdev,
 				    struct device_attribute *attr, char *buf)
 {
-	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 
-	if (edev->cb->get_active)
-		edev->cb->get_active(edev, ecomp);
+	/* The value of ecomp->active may have been updated by edev->cb->set_active(). */
 	return sysfs_emit(buf, "%d\n", ecomp->active);
 }
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 1c630e2c2756..8d09c6d07bf1 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -62,8 +62,6 @@ struct enclosure_component_callbacks {
 	int (*set_fault)(struct enclosure_device *,
 			 struct enclosure_component *,
 			 enum enclosure_component_setting);
-	void (*get_active)(struct enclosure_device *,
-			   struct enclosure_component *);
 	int (*set_active)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_component_setting);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/2] misc: enclosure: remove get_active callback
  2023-05-24 11:12 ` [PATCH v2 1/2] misc: enclosure: remove get_active callback Mariusz Tkaczyk
@ 2023-05-25  9:38   ` Christoph Hellwig
  0 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2023-05-25  9:38 UTC (permalink / raw)
  To: Mariusz Tkaczyk; +Cc: arnd, andriy.shevchenko, linux-kernel, dan.j.williams

On Wed, May 24, 2023 at 01:12:30PM +0200, Mariusz Tkaczyk wrote:
> +	/* The value of ecomp->active may have been updated by edev->cb->set_active(). */

Please don't write overly long comment lines.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] misc: enclosure: update sysfs api
  2023-05-24 11:12 [PATCH v2 0/2] Enclosure interface cleanups Mariusz Tkaczyk
  2023-05-24 11:12 ` [PATCH v2 1/2] misc: enclosure: remove get_active callback Mariusz Tkaczyk
@ 2023-05-24 11:12 ` Mariusz Tkaczyk
  2023-05-25  9:39   ` Christoph Hellwig
  1 sibling, 1 reply; 5+ messages in thread
From: Mariusz Tkaczyk @ 2023-05-24 11:12 UTC (permalink / raw)
  To: arnd; +Cc: andriy.shevchenko, linux-kernel, dan.j.williams

Use DEVICE_ATTR RW and RO macros. Update function names accordingly.
No functional changes intended.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/misc/enclosure.c | 80 +++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 42 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 59704fdb962a..95d70840a88a 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -473,8 +473,8 @@ static const char *const enclosure_type[] = {
 	[ENCLOSURE_COMPONENT_ARRAY_DEVICE] = "array device",
 };
 
-static ssize_t get_component_fault(struct device *cdev,
-				   struct device_attribute *attr, char *buf)
+static ssize_t fault_show(struct device *cdev, struct device_attribute *attr,
+			  char *buf)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -484,9 +484,8 @@ static ssize_t get_component_fault(struct device *cdev,
 	return sysfs_emit(buf, "%d\n", ecomp->fault);
 }
 
-static ssize_t set_component_fault(struct device *cdev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t count)
+static ssize_t fault_store(struct device *cdev, struct device_attribute *attr,
+			   const char *buf, size_t count)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -497,8 +496,10 @@ static ssize_t set_component_fault(struct device *cdev,
 	return count;
 }
 
-static ssize_t get_component_status(struct device *cdev,
-				    struct device_attribute *attr,char *buf)
+static DEVICE_ATTR_RW(fault);
+
+static ssize_t status_show(struct device *cdev, struct device_attribute *attr,
+			   char *buf)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -508,9 +509,8 @@ static ssize_t get_component_status(struct device *cdev,
 	return sysfs_emit(buf, "%s\n", enclosure_status[ecomp->status]);
 }
 
-static ssize_t set_component_status(struct device *cdev,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
+static ssize_t status_store(struct device *cdev, struct device_attribute *attr,
+			    const char *buf, size_t count)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -531,8 +531,10 @@ static ssize_t set_component_status(struct device *cdev,
 		return -EINVAL;
 }
 
-static ssize_t get_component_active(struct device *cdev,
-				    struct device_attribute *attr, char *buf)
+static DEVICE_ATTR_RW(status);
+
+static ssize_t active_show(struct device *cdev, struct device_attribute *attr,
+			   char *buf)
 {
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 
@@ -540,9 +542,8 @@ static ssize_t get_component_active(struct device *cdev,
 	return sysfs_emit(buf, "%d\n", ecomp->active);
 }
 
-static ssize_t set_component_active(struct device *cdev,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
+static ssize_t active_store(struct device *cdev, struct device_attribute *attr,
+			    const char *buf, size_t count)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -553,8 +554,10 @@ static ssize_t set_component_active(struct device *cdev,
 	return count;
 }
 
-static ssize_t get_component_locate(struct device *cdev,
-				    struct device_attribute *attr, char *buf)
+static DEVICE_ATTR_RW(active);
+
+static ssize_t locate_show(struct device *cdev, struct device_attribute *attr,
+			   char *buf)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -564,9 +567,8 @@ static ssize_t get_component_locate(struct device *cdev,
 	return sysfs_emit(buf, "%d\n", ecomp->locate);
 }
 
-static ssize_t set_component_locate(struct device *cdev,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
+static ssize_t locate_store(struct device *cdev, struct device_attribute *attr,
+			    const char *buf, size_t count)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -577,9 +579,10 @@ static ssize_t set_component_locate(struct device *cdev,
 	return count;
 }
 
-static ssize_t get_component_power_status(struct device *cdev,
-					  struct device_attribute *attr,
-					  char *buf)
+static DEVICE_ATTR_RW(locate);
+
+static ssize_t power_status_show(struct device *cdev,
+				 struct device_attribute *attr, char *buf)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -594,9 +597,9 @@ static ssize_t get_component_power_status(struct device *cdev,
 	return sysfs_emit(buf, "%s\n", ecomp->power_status ? "on" : "off");
 }
 
-static ssize_t set_component_power_status(struct device *cdev,
-					  struct device_attribute *attr,
-					  const char *buf, size_t count)
+static ssize_t power_status_store(struct device *cdev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
@@ -616,16 +619,20 @@ static ssize_t set_component_power_status(struct device *cdev,
 	return count;
 }
 
-static ssize_t get_component_type(struct device *cdev,
-				  struct device_attribute *attr, char *buf)
+static DEVICE_ATTR_RW(power_status);
+
+static ssize_t type_show(struct device *cdev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 
 	return sysfs_emit(buf, "%s\n", enclosure_type[ecomp->type]);
 }
 
-static ssize_t get_component_slot(struct device *cdev,
-				  struct device_attribute *attr, char *buf)
+static DEVICE_ATTR_RO(type);
+
+static ssize_t slot_show(struct device *cdev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
 	int slot;
@@ -639,18 +646,7 @@ static ssize_t get_component_slot(struct device *cdev,
 	return sysfs_emit(buf, "%d\n", slot);
 }
 
-static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
-		    set_component_fault);
-static DEVICE_ATTR(status, S_IRUGO | S_IWUSR, get_component_status,
-		   set_component_status);
-static DEVICE_ATTR(active, S_IRUGO | S_IWUSR, get_component_active,
-		   set_component_active);
-static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
-		   set_component_locate);
-static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR, get_component_power_status,
-		   set_component_power_status);
-static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
-static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
+static DEVICE_ATTR_RO(slot);
 
 static struct attribute *enclosure_component_attrs[] = {
 	&dev_attr_fault.attr,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] misc: enclosure: update sysfs api
  2023-05-24 11:12 ` [PATCH v2 2/2] misc: enclosure: update sysfs api Mariusz Tkaczyk
@ 2023-05-25  9:39   ` Christoph Hellwig
  0 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2023-05-25  9:39 UTC (permalink / raw)
  To: Mariusz Tkaczyk; +Cc: arnd, andriy.shevchenko, linux-kernel, dan.j.williams

On Wed, May 24, 2023 at 01:12:31PM +0200, Mariusz Tkaczyk wrote:
> Use DEVICE_ATTR RW and RO macros. Update function names accordingly.
> No functional changes intended.

The subject looks a bit, I'd say something like:

"enclosure: use DEVICE_ATTR_RW* macros"

instead.

The actual changes look good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-05-25  9:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-24 11:12 [PATCH v2 0/2] Enclosure interface cleanups Mariusz Tkaczyk
2023-05-24 11:12 ` [PATCH v2 1/2] misc: enclosure: remove get_active callback Mariusz Tkaczyk
2023-05-25  9:38   ` Christoph Hellwig
2023-05-24 11:12 ` [PATCH v2 2/2] misc: enclosure: update sysfs api Mariusz Tkaczyk
2023-05-25  9:39   ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox