On 11/01/2024 15:54, Miroslav Franc wrote:
> Jan Höppner <hoeppner@xxxxxxxxxxxxx> writes:
>
>> On 10/01/2024 17:01, Miroslav Franc wrote:
>>> Once the discipline is associated with the device, deleting the device
>>> takes care of decrementing the module's refcount. Doing it manually on
>>> this error path causes refcount to artificially decrease on each error
>>> while it should just stay the same.
>>>
>>> Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
>>> Signed-off-by: Miroslav Franc <mfranc@xxxxxxx>
>>> ---
>>> drivers/s390/block/dasd.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
>>> index 833cfab7d877..739da1c2b71f 100644
>>> --- a/drivers/s390/block/dasd.c
>>> +++ b/drivers/s390/block/dasd.c
>>> @@ -3546,8 +3546,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
>>> if (rc) {
>>> pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
>>> dev_name(&cdev->dev), discipline->name, rc);
>>> - module_put(discipline->owner);
>>> - module_put(base_discipline->owner);
>>
>> Good catch. I think there is one more line above this part that should
>> also be removed:
>>
>> if (!try_module_get(discipline->owner)) {
>> module_put(base_discipline->owner); <---
>> dasd_delete_device(device);
>> return -EINVAL;
>> }
>
> Oh, I was under impression that the following line is necessary for
> dasd_delete_device to work that way.
You're absolutely right, I've missed that part, sorry.
>
> device->base_discipline = base_discipline;
>
> I could move it before the if statement before removing module_put from
> it. Does it make sense?
Yes that makes sense. That way the (decrement) refcounting is entirely done
via the dasd_delete_device() function. I'll take your patch as suggested
below. Thanks a lot!
>
>>
>> Can you add it to the patch? Thanks!
>>
>>> dasd_delete_device(device);
>>> return rc;
>>> }
>>>
>
> Once the discipline is associated with the device, deleting the device
> takes care of decrementing the module's refcount. Doing it manually on
> this error path causes refcount to artificially decrease on each error
> while it should just stay the same.
>
> Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
> Signed-off-by: Miroslav Franc <mfranc@xxxxxxx>
> ---
> drivers/s390/block/dasd.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 833cfab7d877..8e453454c271 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -3533,12 +3533,11 @@ int dasd_generic_set_online(struct ccw_device *cdev,
> dasd_delete_device(device);
> return -EINVAL;
> }
> + device->base_discipline = base_discipline;
> if (!try_module_get(discipline->owner)) {
> - module_put(base_discipline->owner);
> dasd_delete_device(device);
> return -EINVAL;
> }
> - device->base_discipline = base_discipline;
> device->discipline = discipline;
>
> /* check_device will allocate block device if necessary */
> @@ -3546,8 +3545,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
> if (rc) {
> pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
> dev_name(&cdev->dev), discipline->name, rc);
> - module_put(discipline->owner);
> - module_put(base_discipline->owner);
> dasd_delete_device(device);
> return rc;
> }
