[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a5759f89-0617-29a1-26bd-0ed9a4bc41b8@wanadoo.fr>
Date: Mon, 17 May 2021 13:24:42 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: arnd@...db.de, gregkh@...uxfoundation.org,
mihai.carabas@...cle.com, pizhenwei@...edance.com,
pbonzini@...hat.com, linqiheng@...wei.com,
linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/2] misc/pvpanic: Fix error handling in
'pvpanic_pci_probe()'
Le 17/05/2021 à 12:34, Andy Shevchenko a écrit :
> On Mon, May 17, 2021 at 12:02:24PM +0200, Christophe JAILLET wrote:
>> Le 17/05/2021 à 10:01, Andy Shevchenko a écrit :
>>> On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
>>>> There is no error handling path in the probe function.
>>>> Switch to managed resource so that errors in the probe are handled easily
>>>> and simplify the remove function accordingly.
>>>
>>> Yes, that's what I suggested earlier to another contributor.
>>>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>>>
>>> Thanks!
>>>
>>> P.S. You may consider the following things as well:
>>> 1) converting to use pci_set_drvdata() / pci_get_drvdata()
>>
>> I can send a patch for that if you want.
>> But it looks really low value for a driver that is already very short and
>> clean.
>
> Yep, that's why 2) below came to my mind (then you will remove drvdata call).
>
>>> 2) providing devm_pvpanic_probe() [via devm_add_action() /
>>> devm_add_action_or_reset()]
>>
>> I don't follow you here.
>> The goal would be to avoid the remove function and "record" the needed
>> action directly in the probe?
>>
>> If this is it, I would only see an unusual pattern and a harder to follow
>> logic.
>
>> Did I miss something?
>> What would be the benefit?
>
> First of all it's a usual pattern when one, often used in ->probe(), function
> gains its devm variant. See, for example, `return devm_gpiochip_add_data(...);`
> used in the code.
>
> Benefit is to have everything under managed resources and yes, no ->remove()
> will be needed in the individual drivers.
>
> But it's up to you. It was just a proposal that you may simply refuse to follow,
> it's fine.
>
Ok, I'll propose something when/if my first patches reach -next.
I now better see your point. I first read devm_pvpanic_pci_probe (i.e.
with pci inside), instead of devm_pvpanic_probe.
CJ
Powered by blists - more mailing lists