| Message ID | 20210902174105.2418771-7-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: 5th batch of add_disk() error handling conversions | expand |
On 02/09/2021 19:41, Luis Chamberlain wrote: > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/s390/block/dasd_genhd.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c > index fa966e0db6ca..ba07022283bc 100644 > --- a/drivers/s390/block/dasd_genhd.c > +++ b/drivers/s390/block/dasd_genhd.c > @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) > { > struct gendisk *gdp; > struct dasd_device *base; > - int len; > + int len, rc; > > /* Make sure the minor for this device exists. */ > base = block->base; > @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) > dasd_add_link_to_gendisk(gdp, base); > block->gdp = gdp; > set_capacity(block->gdp, 0); > - device_add_disk(&base->cdev->dev, block->gdp, NULL); > + > + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); > + if (rc) > + return rc; > + I think, just like with some of the other changes, there is some cleanup required before returning. I'll prepare a patch and come back to you. > return 0; > } > >
On Mon, Sep 13, 2021 at 10:17:48AM +0200, Jan H??ppner wrote: > I think, just like with some of the other changes, there is some > cleanup required before returning. I'll prepare a patch and > come back to you. If you are touching the dasd probe path anyway, can you look insto switching to use blk_mq_alloc_disk as well? Right now dasd allocates the request_queue and gendisk separately in different stages of initialization, but unlike SCSI which needs to probe using just the request_queue I can't find a good reason for that.
On 13/09/2021 10:42, Christoph Hellwig wrote: > On Mon, Sep 13, 2021 at 10:17:48AM +0200, Jan H??ppner wrote: >> I think, just like with some of the other changes, there is some >> cleanup required before returning. I'll prepare a patch and >> come back to you. > > If you are touching the dasd probe path anyway, can you look insto > switching to use blk_mq_alloc_disk as well? Right now dasd allocates > the request_queue and gendisk separately in different stages of > initialization, but unlike SCSI which needs to probe using just the > request_queue I can't find a good reason for that. > Thanks for the hint. We'll be working on it separately though, as it seems to require a bit more effort from a first glance. We'll send a separate patch (or patchset) soon. regards, Jan
On 13/09/2021 10:17, Jan Höppner wrote: > On 02/09/2021 19:41, Luis Chamberlain wrote: >> We never checked for errors on add_disk() as this function >> returned void. Now that this is fixed, use the shiny new >> error handling. >> >> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> >> --- >> drivers/s390/block/dasd_genhd.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c >> index fa966e0db6ca..ba07022283bc 100644 >> --- a/drivers/s390/block/dasd_genhd.c >> +++ b/drivers/s390/block/dasd_genhd.c >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> { >> struct gendisk *gdp; >> struct dasd_device *base; >> - int len; >> + int len, rc; >> >> /* Make sure the minor for this device exists. */ >> base = block->base; >> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> dasd_add_link_to_gendisk(gdp, base); >> block->gdp = gdp; >> set_capacity(block->gdp, 0); >> - device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + if (rc) >> + return rc; >> + > > I think, just like with some of the other changes, there is some > cleanup required before returning. I'll prepare a patch and > come back to you. > It's actually just one call that is required. The patch should look like this: diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index fa966e0db6ca..80673dbfb1f9 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) { struct gendisk *gdp; struct dasd_device *base; - int len; + int len, rc; /* Make sure the minor for this device exists. */ base = block->base; @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block) dasd_add_link_to_gendisk(gdp, base); block->gdp = gdp; set_capacity(block->gdp, 0); - device_add_disk(&base->cdev->dev, block->gdp, NULL); + + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); + if (rc) { + dasd_gendisk_free(block); + return rc; + } + return 0; } regards, Jan
On Mon, Sep 13, 2021 at 02:19:38PM +0200, Jan Höppner wrote: > On 13/09/2021 10:17, Jan Höppner wrote: > > On 02/09/2021 19:41, Luis Chamberlain wrote: > >> We never checked for errors on add_disk() as this function > >> returned void. Now that this is fixed, use the shiny new > >> error handling. > >> > >> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > >> --- > >> drivers/s390/block/dasd_genhd.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c > >> index fa966e0db6ca..ba07022283bc 100644 > >> --- a/drivers/s390/block/dasd_genhd.c > >> +++ b/drivers/s390/block/dasd_genhd.c > >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) > >> { > >> struct gendisk *gdp; > >> struct dasd_device *base; > >> - int len; > >> + int len, rc; > >> > >> /* Make sure the minor for this device exists. */ > >> base = block->base; > >> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) > >> dasd_add_link_to_gendisk(gdp, base); > >> block->gdp = gdp; > >> set_capacity(block->gdp, 0); > >> - device_add_disk(&base->cdev->dev, block->gdp, NULL); > >> + > >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); > >> + if (rc) > >> + return rc; > >> + > > > > I think, just like with some of the other changes, there is some > > cleanup required before returning. I'll prepare a patch and > > come back to you. > > > > It's actually just one call that is required. The patch should > look like this: > > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c > index fa966e0db6ca..80673dbfb1f9 100644 > --- a/drivers/s390/block/dasd_genhd.c > +++ b/drivers/s390/block/dasd_genhd.c > @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) > { > struct gendisk *gdp; > struct dasd_device *base; > - int len; > + int len, rc; > > /* Make sure the minor for this device exists. */ > base = block->base; > @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block) > dasd_add_link_to_gendisk(gdp, base); > block->gdp = gdp; > set_capacity(block->gdp, 0); > - device_add_disk(&base->cdev->dev, block->gdp, NULL); > + > + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); > + if (rc) { > + dasd_gendisk_free(block); > + return rc; > + } > + Thanks! Would you like to to fold this fix into my patch and resend eventually? Or will you send a replacement? Luis
On 13/09/2021 18:51, Luis Chamberlain wrote: > On Mon, Sep 13, 2021 at 02:19:38PM +0200, Jan Höppner wrote: >> On 13/09/2021 10:17, Jan Höppner wrote: >>> On 02/09/2021 19:41, Luis Chamberlain wrote: >>>> We never checked for errors on add_disk() as this function >>>> returned void. Now that this is fixed, use the shiny new >>>> error handling. >>>> >>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> >>>> --- >>>> drivers/s390/block/dasd_genhd.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c >>>> index fa966e0db6ca..ba07022283bc 100644 >>>> --- a/drivers/s390/block/dasd_genhd.c >>>> +++ b/drivers/s390/block/dasd_genhd.c >>>> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) >>>> { >>>> struct gendisk *gdp; >>>> struct dasd_device *base; >>>> - int len; >>>> + int len, rc; >>>> >>>> /* Make sure the minor for this device exists. */ >>>> base = block->base; >>>> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) >>>> dasd_add_link_to_gendisk(gdp, base); >>>> block->gdp = gdp; >>>> set_capacity(block->gdp, 0); >>>> - device_add_disk(&base->cdev->dev, block->gdp, NULL); >>>> + >>>> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); >>>> + if (rc) >>>> + return rc; >>>> + >>> >>> I think, just like with some of the other changes, there is some >>> cleanup required before returning. I'll prepare a patch and >>> come back to you. >>> >> >> It's actually just one call that is required. The patch should >> look like this: >> >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c >> index fa966e0db6ca..80673dbfb1f9 100644 >> --- a/drivers/s390/block/dasd_genhd.c >> +++ b/drivers/s390/block/dasd_genhd.c >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> { >> struct gendisk *gdp; >> struct dasd_device *base; >> - int len; >> + int len, rc; >> >> /* Make sure the minor for this device exists. */ >> base = block->base; >> @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> dasd_add_link_to_gendisk(gdp, base); >> block->gdp = gdp; >> set_capacity(block->gdp, 0); >> - device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + if (rc) { >> + dasd_gendisk_free(block); >> + return rc; >> + } >> + > > Thanks! > > Would you like to to fold this fix into my patch and resend eventually? > Or will you send a replacement? > > Luis > I'd be fine with you just taking the changes for your patchset. Once you've resent the whole patchset I'll review it and send the usual ack or r-b. regards, Jan
diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index fa966e0db6ca..ba07022283bc 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) { struct gendisk *gdp; struct dasd_device *base; - int len; + int len, rc; /* Make sure the minor for this device exists. */ base = block->base; @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) dasd_add_link_to_gendisk(gdp, base); block->gdp = gdp; set_capacity(block->gdp, 0); - device_add_disk(&base->cdev->dev, block->gdp, NULL); + + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); + if (rc) + return rc; + return 0; }
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/dasd_genhd.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)