diff mbox series

[6/9] s390/block/dasd_genhd: add error handling support for add_disk()

Message ID 20210902174105.2418771-7-mcgrof@kernel.org
State New
Headers show
Series block: 5th batch of add_disk() error handling conversions | expand

Commit Message

Luis Chamberlain Sept. 2, 2021, 5:41 p.m. UTC
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(-)

Comments

Jan Höppner Sept. 13, 2021, 8:17 a.m. UTC | #1
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;

>  }

>  

>
Christoph Hellwig Sept. 13, 2021, 8:42 a.m. UTC | #2
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.
Jan Höppner Sept. 13, 2021, 12:15 p.m. UTC | #3
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
Jan Höppner Sept. 13, 2021, 12:19 p.m. UTC | #4
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
Luis Chamberlain Sept. 13, 2021, 4:51 p.m. UTC | #5
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
Jan Höppner Sept. 15, 2021, 2:57 p.m. UTC | #6
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 mbox series

Patch

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;
 }