linux-kernel - Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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