linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Steffen Maier <maier@linux.ibm.com>
To: bvanassche@acm.org, martin.petersen@oracle.com,
	jejb@linux.ibm.com, linux-scsi@vger.kernel.org
Cc: gregkh@linuxfoundation.org, maier@linux.ibm.com,
	bblock@linux.ibm.com, linux-next@vger.kernel.org,
	linux-s390@vger.kernel.org
Subject: [PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device
Date: Mon, 25 Oct 2021 00:16:20 +0200	[thread overview]
Message-ID: <20211024221620.760160-1-maier@linux.ibm.com> (raw)
In-Reply-To: <b5e69621-e2ee-750a-e542-a27aaa9293e5@acm.org>

v4.17 commit 86b87cde0b55 ("scsi: core: host template attribute groups")
introduced explicit sysfs_create_groups() in scsi_sysfs_add_sdev()
and sysfs_remove_groups() in __scsi_remove_device(), both for sdev_gendev,
based on a new field const struct attribute_group **sdev_groups
of struct scsi_host_template.

Commit 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier")
removed above explicit (de)registration of scsi_device attribute groups.
It also converted all scsi_device attributes and attribute_groups to
end up in a new field const struct attribute_group *gendev_attr_groups[6]
of struct scsi_device. However, that new field was not used anywhere.

Surprisingly, this only caused missing LLDD specific scsi_device sysfs
attributes. Whereas, scsi core attributes from scsi_sdev_attr_groups
did continue to exist because of scsi_dev_type.groups.

Fix it by assigning the pointer of that new field to the groups
field of sdev_gendev so the driver core gets our LLDD groups.
Just like scsi_host_alloc() was changed by the same commit,
although scsi_host_alloc() already had assigned something to
shost_dev.groups so the necessary change was more obvious there.

We separate scsi core attibutes from LLDD specific attributes.
Hence, we keep the initializing assignment scsi_dev_type =
{ .groups = scsi_sdev_attr_groups, } as this takes care of core
attributes. Without the separation, it would cause attribute double
registration due to scsi_dev_type.groups and sdev_gendev.groups.

Since we now no longer have an index offset between left and right
hand side of the assignment in the loop, one counter variable suffices.

Alternative approaches ruled out:
Assigning gendev_attr_groups to sdev_dev has no visible effect.
Assigning sdev->gendev_attr_groups to scsi_dev_type.groups
caused scsi_device of all scsi host types to get LLDD specific
attributes of the LLDD for which the last sdev alloc happened to occur,
as that overwrote scsi_dev_type.groups,
e.g. scsi_debug had zfcp-specific scsi_device attributes.

Signed-off-by: Steffen Maier <maier@linux.ibm.com>
Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier")
---

Changes in v2:
* integrated Bart's feedback of updating the comment for
  the gendev_attr_groups declaration to match the code change
* in that spirit also adapted the vector size of that field
* eliminated the now unnecessary second loop counter 'j'

 drivers/scsi/scsi_sysfs.c  | 12 ++++++------
 include/scsi/scsi_device.h |  7 +++----
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index c26f0e29e8cd..d73e84e1cb37 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1571,7 +1571,7 @@ static struct device_type scsi_dev_type = {
 
 void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 {
-	int i, j = 0;
+	int i = 0;
 	unsigned long flags;
 	struct Scsi_Host *shost = sdev->host;
 	struct scsi_host_template *hostt = shost->hostt;
@@ -1583,15 +1583,15 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 	scsi_enable_async_suspend(&sdev->sdev_gendev);
 	dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu",
 		     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
-	sdev->gendev_attr_groups[j++] = &scsi_sdev_attr_group;
+	sdev->sdev_gendev.groups = sdev->gendev_attr_groups;
 	if (hostt->sdev_groups) {
 		for (i = 0; hostt->sdev_groups[i] &&
-			     j < ARRAY_SIZE(sdev->gendev_attr_groups);
-		     i++, j++) {
-			sdev->gendev_attr_groups[j] = hostt->sdev_groups[i];
+			     i < ARRAY_SIZE(sdev->gendev_attr_groups);
+		     i++) {
+			sdev->gendev_attr_groups[i] = hostt->sdev_groups[i];
 		}
 	}
-	WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups));
+	WARN_ON_ONCE(i >= ARRAY_SIZE(sdev->gendev_attr_groups));
 
 	device_initialize(&sdev->sdev_dev);
 	sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index b1e9b3bd3a60..b6f0d031217e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -226,11 +226,10 @@ struct scsi_device {
 	struct device		sdev_gendev,
 				sdev_dev;
 	/*
-	 * The array size 6 provides space for one attribute group for the
-	 * SCSI core, four attribute groups defined by SCSI LLDs and one
-	 * terminating NULL pointer.
+	 * The array size 5 provides space for four attribute groups
+	 * defined by SCSI LLDs and one terminating NULL pointer.
 	 */
-	const struct attribute_group *gendev_attr_groups[6];
+	const struct attribute_group *gendev_attr_groups[5];
 
 	struct execute_work	ew; /* used to get process context on put */
 	struct work_struct	requeue_work;
-- 
2.25.1


  reply	other threads:[~2021-10-24 22:16 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-12 23:35 [PATCH v4 00/46] Register SCSI sysfs attributes earlier Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 01/46] scsi: core: Register " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 02/46] ata: Switch to attribute groups Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 03/46] firewire: sbp2: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 04/46] RDMA/srp: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 05/46] scsi: message: fusion: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 06/46] scsi: zfcp: " Bart Van Assche
2021-10-13 17:34   ` Benjamin Block
2021-10-12 23:35 ` [PATCH v4 07/46] scsi: 3w-9xxx: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 08/46] scsi: 3w-sas: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 09/46] scsi: 3w-xxxx: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 10/46] scsi: 53c700: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 11/46] scsi: aacraid: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 12/46] scsi: arcmsr: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 13/46] scsi: be2iscsi: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 14/46] scsi: bfa: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 15/46] scsi: bnx2fc: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 16/46] scsi: bnx2i: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 17/46] scsi: csiostor: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 18/46] scsi: cxlflash: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 19/46] scsi: fnic: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 20/46] scsi: hisi_sas: " Bart Van Assche
2021-10-13  8:22   ` John Garry
2021-10-12 23:35 ` [PATCH v4 21/46] scsi: hpsa: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 22/46] scsi: hptiop: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 23/46] scsi: ibmvscsi: " Bart Van Assche
2021-10-14 18:30   ` Tyrel Datwyler
2021-10-12 23:35 ` [PATCH v4 24/46] scsi: ibmvfc: " Bart Van Assche
2021-10-14 18:31   ` Tyrel Datwyler
2021-10-12 23:35 ` [PATCH v4 25/46] scsi: ipr: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 26/46] scsi: isci: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 27/46] scsi: lpfc: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 28/46] scsi: megaraid: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 29/46] scsi: mpt3sas: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 30/46] scsi: mvsas: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 31/46] scsi: myrb: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 32/46] scsi: myrs: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 33/46] scsi: ncr53c8xx: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 34/46] scsi: sym53c500_cs: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 35/46] scsi: pm8001: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 36/46] scsi: pmcraid: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 37/46] scsi: qedf: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 38/46] scsi: qedi: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 39/46] scsi: qla2xxx: Remove a declaration Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 40/46] scsi: qla2xxx: Switch to attribute groups Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 41/46] scsi: qla4xxx: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 42/46] scsi: smartpqi: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 43/46] scsi: snic: " Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 44/46] scsi: unisys: Remove the shost_attrs member Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 45/46] scsi: usb: Switch to attribute groups Bart Van Assche
2021-10-12 23:35 ` [PATCH v4 46/46] scsi: core: Remove two host template members that are no longer used Bart Van Assche
2021-10-17  1:48 ` [PATCH v4 00/46] Register SCSI sysfs attributes earlier Martin K. Petersen
2021-10-21  3:42 ` Martin K. Petersen
2021-10-23 20:54   ` Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier] Steffen Maier
2021-10-24  0:39     ` Steffen Maier
2021-10-24  2:55       ` Bart Van Assche
2021-10-24 11:18       ` [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device Steffen Maier
2021-10-24 21:25         ` Bart Van Assche
2021-10-24 22:16           ` Steffen Maier [this message]
2021-10-25  9:23             ` [PATCH v2] " Julian Wiedmann
2021-10-25 16:00               ` Bart Van Assche
2021-10-26  1:42               ` [PATCH v3] " Steffen Maier
2021-10-26 10:00                 ` Benjamin Block
2021-10-26 21:48                   ` Bart Van Assche
2021-10-29 17:55                     ` Benjamin Block
2021-10-26 18:31                 ` Bart Van Assche
2021-10-29 17:56                 ` Benjamin Block

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211024221620.760160-1-maier@linux.ibm.com \
    --to=maier@linux.ibm.com \
    --cc=bblock@linux.ibm.com \
    --cc=bvanassche@acm.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jejb@linux.ibm.com \
    --cc=linux-next@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --subject='Re: [PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).