| Message ID | 20221104092734.4110745-1-yangyingliang@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: libsas: fix error handing in sas_ex_discover_expander() | expand |
On 04/11/2022 09:27, Yang Yingliang wrote: > Check return value of sas_port_alloc() and sas_port_add() and handle > the error. If they fail, free the device and port, then returns NULL > instead of using BUG_ON(). > > Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> I already sent a patch to handle all errors here: https://lore.kernel.org/linux-scsi/1666693096-180008-7-git-send-email-john.garry@huawei.com/ Do you actually have a cascaded expander setup to test this? > --- > drivers/scsi/libsas/sas_expander.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c > index 5ce251830104..88b8b955d533 100644 > --- a/drivers/scsi/libsas/sas_expander.c > +++ b/drivers/scsi/libsas/sas_expander.c > @@ -935,8 +935,17 @@ static struct domain_device *sas_ex_discover_expander( > return NULL; > > phy->port = sas_port_alloc(&parent->rphy->dev, phy_id); > - /* FIXME: better error handling */ > - BUG_ON(sas_port_add(phy->port) != 0); > + if (!phy->port) { > + sas_put_device(child); > + return NULL; > + } > + > + if (sas_port_add(phy->port)) { > + sas_port_free(phy->port); > + phy->port = NULL; > + sas_put_device(child); > + return NULL; > + } > > > switch (phy->attached_dev_type) {
On 2022/11/7 17:39, John Garry wrote: > On 04/11/2022 09:27, Yang Yingliang wrote: >> Check return value of sas_port_alloc() and sas_port_add() and handle >> the error. If they fail, free the device and port, then returns NULL >> instead of using BUG_ON(). >> >> Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver") >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > I already sent a patch to handle all errors here: > > https://lore.kernel.org/linux-scsi/1666693096-180008-7-git-send-email-john.garry@huawei.com/ > Your patch is better to handle all of these, this patch can be dropped. Thanks, Yang > > Do you actually have a cascaded expander setup to test this? > >> --- >> drivers/scsi/libsas/sas_expander.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_expander.c >> b/drivers/scsi/libsas/sas_expander.c >> index 5ce251830104..88b8b955d533 100644 >> --- a/drivers/scsi/libsas/sas_expander.c >> +++ b/drivers/scsi/libsas/sas_expander.c >> @@ -935,8 +935,17 @@ static struct domain_device >> *sas_ex_discover_expander( >> return NULL; >> phy->port = sas_port_alloc(&parent->rphy->dev, phy_id); >> - /* FIXME: better error handling */ >> - BUG_ON(sas_port_add(phy->port) != 0); >> + if (!phy->port) { >> + sas_put_device(child); >> + return NULL; >> + } >> + >> + if (sas_port_add(phy->port)) { >> + sas_port_free(phy->port); >> + phy->port = NULL; >> + sas_put_device(child); >> + return NULL; > + } >> switch (phy->attached_dev_type) { > > .
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c index 5ce251830104..88b8b955d533 100644 --- a/drivers/scsi/libsas/sas_expander.c +++ b/drivers/scsi/libsas/sas_expander.c @@ -935,8 +935,17 @@ static struct domain_device *sas_ex_discover_expander( return NULL; phy->port = sas_port_alloc(&parent->rphy->dev, phy_id); - /* FIXME: better error handling */ - BUG_ON(sas_port_add(phy->port) != 0); + if (!phy->port) { + sas_put_device(child); + return NULL; + } + + if (sas_port_add(phy->port)) { + sas_port_free(phy->port); + phy->port = NULL; + sas_put_device(child); + return NULL; + } switch (phy->attached_dev_type) {
Check return value of sas_port_alloc() and sas_port_add() and handle the error. If they fail, free the device and port, then returns NULL instead of using BUG_ON(). Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/scsi/libsas/sas_expander.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)