| Message ID | 20220225055621.410896-1-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: libsas: cleanup sas_form_port() | expand |
On 25/02/2022 05:56, 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(). 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> > --- > drivers/scsi/libsas/sas_port.c | 76 ++++++++++++++++++++-------------- > 1 file changed, 45 insertions(+), 31 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c > index 67b429dcf1ff..90bd1666f888 100644 > --- a/drivers/scsi/libsas/sas_port.c > +++ b/drivers/scsi/libsas/sas_port.c > @@ -67,6 +67,39 @@ static void sas_resume_port(struct asd_sas_phy *phy) > sas_discover_event(port, DISCE_RESUME); > } > > +static struct domain_device *__sas_form_port(struct asd_sas_port *port, > + struct asd_sas_phy *phy, > + bool wideport) How about a more obvious name, like "sas_form_port_add_phy()"? And returning a domain_device is strange. We don't assign/evaluate that in this function. Instead I think that you may just use port->port_dev outside later, right? > +{ > + struct domain_device *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); nit: no need for () > + > + 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); > + } > + > + return port_dev; > Thanks, John
On 2/25/22 18:54, John Garry wrote: > On 25/02/2022 05:56, 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(). 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> >> --- >> drivers/scsi/libsas/sas_port.c | 76 ++++++++++++++++++++-------------- >> 1 file changed, 45 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c >> index 67b429dcf1ff..90bd1666f888 100644 >> --- a/drivers/scsi/libsas/sas_port.c >> +++ b/drivers/scsi/libsas/sas_port.c >> @@ -67,6 +67,39 @@ static void sas_resume_port(struct asd_sas_phy *phy) >> sas_discover_event(port, DISCE_RESUME); >> } >> >> +static struct domain_device *__sas_form_port(struct asd_sas_port *port, >> + struct asd_sas_phy *phy, >> + bool wideport) > > How about a more obvious name, like "sas_form_port_add_phy()"? > > And returning a domain_device is strange. We don't assign/evaluate that > in this function. Instead I think that you may just use port->port_dev > outside later, right? OK. Will send V2. Thanks for the review. > >> +{ >> + struct domain_device *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); > > nit: no need for () > >> + >> + 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); >> + } >> + >> + return port_dev; >> > Thanks, > John
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c index 67b429dcf1ff..90bd1666f888 100644 --- a/drivers/scsi/libsas/sas_port.c +++ b/drivers/scsi/libsas/sas_port.c @@ -67,6 +67,39 @@ static void sas_resume_port(struct asd_sas_phy *phy) sas_discover_event(port, DISCE_RESUME); } +static struct domain_device *__sas_form_port(struct asd_sas_port *port, + struct asd_sas_phy *phy, + bool wideport) +{ + struct domain_device *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 (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); + } + + return port_dev; +} + /** * sas_form_port - add this phy to a port * @phy: the phy of interest @@ -79,7 +112,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 +143,8 @@ 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 = __sas_form_port(port, phy, true); + spin_unlock(&port->phy_list_lock); break; } spin_unlock(&port->phy_list_lock); @@ -122,40 +155,21 @@ 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 = __sas_form_port(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) {
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(). 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> --- drivers/scsi/libsas/sas_port.c | 76 ++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 31 deletions(-)