diff mbox series

[v2] scsi: libsas: cleanup sas_form_port()

Message ID 20220228094857.557329-1-damien.lemoal@opensource.wdc.com (mailing list archive)
State Accepted
Headers show
Series [v2] scsi: libsas: cleanup sas_form_port() | expand

Commit Message

Damien Le Moal Feb. 28, 2022, 9:48 a.m. UTC
Sparse throws a warning about context imbalance ("different lock
contexts for basic block") in sas_form_port() as it gets confused with
the fact that a port is locked within one of the 2 search loop and
unlocked afterward outside of the search loops once the phy is added to
the port. Since this code is not easy to follow, improve it by factoring
out the code adding the phy to the port once the port is locked into the
helper function sas_form_port_add_phy(). This helper can then be called
directly within the port search loops, avoiding confusion and clearing
the sparse warning.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
Changes from v1:
* Renamed helper function to sas_form_port_add_phy()

 drivers/scsi/libsas/sas_port.c | 73 +++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 31 deletions(-)

Comments

John Garry Feb. 28, 2022, 10:08 a.m. UTC | #1
On 28/02/2022 09:48, Damien Le Moal wrote:
> Sparse throws a warning about context imbalance ("different lock
> contexts for basic block") in sas_form_port() as it gets confused with
> the fact that a port is locked within one of the 2 search loop and
> unlocked afterward outside of the search loops once the phy is added to
> the port. Since this code is not easy to follow, improve it by factoring
> out the code adding the phy to the port once the port is locked into the
> helper function sas_form_port_add_phy(). This helper can then be called
> directly within the port search loops, avoiding confusion and clearing
> the sparse warning.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: John Garry <john.garry@huawei.com>
Martin K. Petersen March 2, 2022, 4:09 a.m. UTC | #2
Damien,

> Sparse throws a warning about context imbalance ("different lock
> contexts for basic block") in sas_form_port() as it gets confused with
> the fact that a port is locked within one of the 2 search loop and
> unlocked afterward outside of the search loops once the phy is added
> to the port. Since this code is not easy to follow, improve it by
> factoring out the code adding the phy to the port once the port is
> locked into the helper function sas_form_port_add_phy(). This helper
> can then be called directly within the port search loops, avoiding
> confusion and clearing the sparse warning.

Applied to 5.18/scsi-staging, thanks!
Martin K. Petersen March 9, 2022, 4:14 a.m. UTC | #3
On Mon, 28 Feb 2022 18:48:57 +0900, Damien Le Moal wrote:

> Sparse throws a warning about context imbalance ("different lock
> contexts for basic block") in sas_form_port() as it gets confused with
> the fact that a port is locked within one of the 2 search loop and
> unlocked afterward outside of the search loops once the phy is added to
> the port. Since this code is not easy to follow, improve it by factoring
> out the code adding the phy to the port once the port is locked into the
> helper function sas_form_port_add_phy(). This helper can then be called
> directly within the port search loops, avoiding confusion and clearing
> the sparse warning.
> 
> [...]

Applied to 5.18/scsi-queue, thanks!

[1/1] scsi: libsas: cleanup sas_form_port()
      https://git.kernel.org/mkp/scsi/c/32698c955295
diff mbox series

Patch

diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index 67b429dcf1ff..11599c0e3fc3 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -67,6 +67,34 @@  static void sas_resume_port(struct asd_sas_phy *phy)
 	sas_discover_event(port, DISCE_RESUME);
 }
 
+static void sas_form_port_add_phy(struct asd_sas_port *port,
+				  struct asd_sas_phy *phy, bool wideport)
+{
+	list_add_tail(&phy->port_phy_el, &port->phy_list);
+	sas_phy_set_target(phy, port->port_dev);
+	phy->port = port;
+	port->num_phys++;
+	port->phy_mask |= (1U << phy->id);
+
+	if (wideport)
+		pr_debug("phy%d matched wide port%d\n", phy->id,
+			 port->id);
+	else
+		memcpy(port->sas_addr, phy->sas_addr, SAS_ADDR_SIZE);
+
+	if (*(u64 *)port->attached_sas_addr == 0) {
+		port->class = phy->class;
+		memcpy(port->attached_sas_addr, phy->attached_sas_addr,
+		       SAS_ADDR_SIZE);
+		port->iproto = phy->iproto;
+		port->tproto = phy->tproto;
+		port->oob_mode = phy->oob_mode;
+		port->linkrate = phy->linkrate;
+	} else {
+		port->linkrate = max(port->linkrate, phy->linkrate);
+	}
+}
+
 /**
  * sas_form_port - add this phy to a port
  * @phy: the phy of interest
@@ -79,7 +107,7 @@  static void sas_form_port(struct asd_sas_phy *phy)
 	int i;
 	struct sas_ha_struct *sas_ha = phy->ha;
 	struct asd_sas_port *port = phy->port;
-	struct domain_device *port_dev;
+	struct domain_device *port_dev = NULL;
 	struct sas_internal *si =
 		to_sas_internal(sas_ha->core.shost->transportt);
 	unsigned long flags;
@@ -110,8 +138,9 @@  static void sas_form_port(struct asd_sas_phy *phy)
 		if (*(u64 *) port->sas_addr &&
 		    phy_is_wideport_member(port, phy) && port->num_phys > 0) {
 			/* wide port */
-			pr_debug("phy%d matched wide port%d\n", phy->id,
-				 port->id);
+			port_dev = port->port_dev;
+			sas_form_port_add_phy(port, phy, true);
+			spin_unlock(&port->phy_list_lock);
 			break;
 		}
 		spin_unlock(&port->phy_list_lock);
@@ -122,40 +151,22 @@  static void sas_form_port(struct asd_sas_phy *phy)
 			port = sas_ha->sas_port[i];
 			spin_lock(&port->phy_list_lock);
 			if (*(u64 *)port->sas_addr == 0
-				&& port->num_phys == 0) {
-				memcpy(port->sas_addr, phy->sas_addr,
-					SAS_ADDR_SIZE);
+			    && port->num_phys == 0) {
+				port_dev = port->port_dev;
+				sas_form_port_add_phy(port, phy, false);
+				spin_unlock(&port->phy_list_lock);
 				break;
 			}
 			spin_unlock(&port->phy_list_lock);
 		}
-	}
 
-	if (i >= sas_ha->num_phys) {
-		pr_err("%s: couldn't find a free port, bug?\n", __func__);
-		spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags);
-		return;
+		if (i >= sas_ha->num_phys) {
+			pr_err("%s: couldn't find a free port, bug?\n",
+			       __func__);
+			spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags);
+			return;
+		}
 	}
-
-	/* add the phy to the port */
-	port_dev = port->port_dev;
-	list_add_tail(&phy->port_phy_el, &port->phy_list);
-	sas_phy_set_target(phy, port_dev);
-	phy->port = port;
-	port->num_phys++;
-	port->phy_mask |= (1U << phy->id);
-
-	if (*(u64 *)port->attached_sas_addr == 0) {
-		port->class = phy->class;
-		memcpy(port->attached_sas_addr, phy->attached_sas_addr,
-		       SAS_ADDR_SIZE);
-		port->iproto = phy->iproto;
-		port->tproto = phy->tproto;
-		port->oob_mode = phy->oob_mode;
-		port->linkrate = phy->linkrate;
-	} else
-		port->linkrate = max(port->linkrate, phy->linkrate);
-	spin_unlock(&port->phy_list_lock);
 	spin_unlock_irqrestore(&sas_ha->phy_port_lock, flags);
 
 	if (!port->port) {