[2/3] misc: enclosure, ses: simplify some get callbacks

Commit Message

Mariusz Tkaczyk Nov. 17, 2022, 4:34 p.m. UTC

Remove active, status, fault and locate variables from
enclosure_component struct. Return then directly.
No functional changes intended.

Signed-off-by: Mariusz Tkaczyk <[e-mail]>
---
 drivers/misc/enclosure.c  | 15 +++++++++------
 drivers/scsi/ses.c        | 33 ++++++++++++++++++---------------
 include/linux/enclosure.h | 12 ++++--------
 3 files changed, 31 insertions(+), 29 deletions(-)

Comments

Dan Williams May 4, 2023, 11:58 p.m. UTC | #1

Mariusz Tkaczyk wrote:
> Remove active, status, fault and locate variables from
> enclosure_component struct. Return then directly.
> No functional changes intended.

This looks ok although it's not a clear win on the diffstat. Does this
make the NPEM implementation easier to remove the indirection through
"struct enclosure_component" for reading fresh values? That would help
make the case.

Mariusz Tkaczyk May 5, 2023, 11:45 a.m. UTC | #2

On Thu, 4 May 2023 16:58:35 -0700
Dan Williams <[e-mail]> wrote:

> Mariusz Tkaczyk wrote:
> > Remove active, status, fault and locate variables from
> > enclosure_component struct. Return then directly.
> > No functional changes intended.  
> 
> This looks ok although it's not a clear win on the diffstat. Does this
> make the NPEM implementation easier to remove the indirection through
> "struct enclosure_component" for reading fresh values? That would help
> make the case.

I did that to familiarize better with this API. I determined that those values
can be just returned. They are refreshed every time on read in get_*led*(). I
believed that it makes implementation simpler for reader.

It could save me from some questions, "why not to reuse existing active,
fault, status variables" but it is no clear benefit.
It saves some memory because those variable probably won't be used in
NPEM/_DSM implementation (at least draft I left doesn't not use them).

If you don't see this valuable let me know, I can drop it.

Thanks,
Mariusz

Dan Williams May 5, 2023, 5:33 p.m. UTC | #3

Mariusz Tkaczyk wrote:
> On Thu, 4 May 2023 16:58:35 -0700
> Dan Williams <[e-mail]> wrote:
> 
> > Mariusz Tkaczyk wrote:
> > > Remove active, status, fault and locate variables from
> > > enclosure_component struct. Return then directly.
> > > No functional changes intended.  
> > 
> > This looks ok although it's not a clear win on the diffstat. Does this
> > make the NPEM implementation easier to remove the indirection through
> > "struct enclosure_component" for reading fresh values? That would help
> > make the case.
> 
> I did that to familiarize better with this API. I determined that those values
> can be just returned. They are refreshed every time on read in get_*led*(). I
> believed that it makes implementation simpler for reader.
> 
> It could save me from some questions, "why not to reuse existing active,
> fault, status variables" but it is no clear benefit.
> It saves some memory because those variable probably won't be used in
> NPEM/_DSM implementation (at least draft I left doesn't not use them).
> 
> If you don't see this valuable let me know, I can drop it.

I think the problem with it is that it now requires a get_active()
implementation where one was not needed before, see my comment on
patch3. The attributes to cache the values allows a 'get' method to be
skipped when 'set'+caching is sufficient.

Patch

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index fd0707a8ed79..00f50fd0cc85 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -478,10 +478,11 @@  static ssize_t get_component_fault(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+	int status = 0;
 
 	if (edev->cb->get_fault)
-		edev->cb->get_fault(edev, ecomp);
-	return sysfs_emit(buf, "%d\n", ecomp->fault);
+		status = edev->cb->get_fault(edev, ecomp);
+	return sysfs_emit(buf, "%d\n", status);
 }
 
 static ssize_t set_component_fault(struct device *cdev,
@@ -502,10 +503,11 @@  static ssize_t get_component_status(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+	enum enclosure_status status = 0;
 
 	if (edev->cb->get_status)
-		edev->cb->get_status(edev, ecomp);
-	return sysfs_emit(buf, "%s\n", enclosure_status[ecomp->status]);
+		status = edev->cb->get_status(edev, ecomp);
+	return sysfs_emit(buf, "%s\n", enclosure_status[status]);
 }
 
 static ssize_t set_component_status(struct device *cdev,
@@ -549,10 +551,11 @@  static ssize_t get_component_locate(struct device *cdev,
 {
 	struct enclosure_device *edev = to_enclosure_device(cdev->parent);
 	struct enclosure_component *ecomp = to_enclosure_component(cdev);
+	int status = 0;
 
 	if (edev->cb->get_locate)
-		edev->cb->get_locate(edev, ecomp);
-	return sysfs_emit(buf, "%d\n", ecomp->locate);
+		status = edev->cb->get_locate(edev, ecomp);
+	return sysfs_emit(buf, "%d\n", status);
 }
 
 static ssize_t set_component_locate(struct device *cdev,
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 0a1734f34587..901dc94e5aeb 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -209,13 +209,14 @@  static void ses_get_fault(struct enclosure_device *edev,
 {
 	unsigned char *desc;
 
-	if (!ses_page2_supported(edev)) {
-		ecomp->fault = 0;
-		return;
-	}
+	if (!ses_page2_supported(edev))
+		return 0;
+
 	desc = ses_get_page2_descriptor(edev, ecomp);
 	if (desc)
-		ecomp->fault = (desc[3] & 0x60) >> 4;
+		return (desc[3] & 0x60) >> 4;
+
+	return 0;
 }
 
 static int ses_set_fault(struct enclosure_device *edev,
@@ -255,13 +256,14 @@  static void ses_get_status(struct enclosure_device *edev,
 {
 	unsigned char *desc;
 
-	if (!ses_page2_supported(edev)) {
-		ecomp->status = 0;
-		return;
-	}
+	if (!ses_page2_supported(edev))
+		return 0;
+
 	desc = ses_get_page2_descriptor(edev, ecomp);
 	if (desc)
-		ecomp->status = (desc[0] & 0x0f);
+		return (desc[0] & 0x0f);
+
+	return 0;
 }
 
 static void ses_get_locate(struct enclosure_device *edev,
@@ -269,13 +271,14 @@  static void ses_get_locate(struct enclosure_device *edev,
 {
 	unsigned char *desc;
 
-	if (!ses_page2_supported(edev)) {
-		ecomp->locate = 0;
-		return;
-	}
+	if (!ses_page2_supported(edev))
+		return 0;
+
 	desc = ses_get_page2_descriptor(edev, ecomp);
 	if (desc)
-		ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
+		return (desc[2] & 0x02) ? 1 : 0;
+
+	return 0;
 }
 
 static int ses_set_locate(struct enclosure_device *edev,
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 8d09c6d07bf1..b70e9deef3bc 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -52,12 +52,12 @@  enum enclosure_component_setting {
 struct enclosure_device;
 struct enclosure_component;
 struct enclosure_component_callbacks {
-	void (*get_status)(struct enclosure_device *,
+	int (*get_status)(struct enclosure_device *,
 			     struct enclosure_component *);
 	int (*set_status)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_status);
-	void (*get_fault)(struct enclosure_device *,
+	int (*get_fault)(struct enclosure_device *,
 			  struct enclosure_component *);
 	int (*set_fault)(struct enclosure_device *,
 			 struct enclosure_component *,
@@ -65,8 +65,8 @@  struct enclosure_component_callbacks {
 	int (*set_active)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_component_setting);
-	void (*get_locate)(struct enclosure_device *,
-			   struct enclosure_component *);
+	int (*get_locate)(struct enclosure_device *,
+			  struct enclosure_component *);
 	int (*set_locate)(struct enclosure_device *,
 			  struct enclosure_component *,
 			  enum enclosure_component_setting);
@@ -85,11 +85,7 @@  struct enclosure_component {
 	struct device *dev;
 	enum enclosure_component_type type;
 	int number;
-	int fault;
-	int active;
-	int locate;
 	int slot;
-	enum enclosure_status status;
 	int power_status;
 };