On Sat, Jan 29, 2022 at 09:07:14AM +0200, Linus Torvalds wrote:
On Sat, Jan 29, 2022 at 8:59 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
It's up to you all, if you think the patch is correct, keep it for now.
In the fixed form (ie with Damien's fix for the wrong test polarity),
it's certainly not wrong, and matches a lot of our standard patterns -
including our documentation in
Documentation/driver-api/driver-model/design-patterns.rst
I did a quick visual grep, and all the cases of
devm_kzalloc(..GFP_KERNEL) I grepped for did indeed have that "if
(!..)" error handling pattern for the return value, including other
cases in the ATA subsystem.
That was very much a "quick visual grep" though, so no guarantees, and
I stopped looking after it was so obvious. IOW, it was just a
git grep -1 devm_kzalloc.*GFP_KERNEL
and then looking at the output and saying "yup, they all seem to do
that allocation failure test".
I think that in this case the issue isn't the correctness of the
devm_kzalloc() allocation test itself, but rather the context in which
it's made in:
host = ata_host_alloc(dev, 1);
if (!host)
return -ENOMEM;
ap = host->ports[0];
ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
if (!ap->ops)
return -ENOMEM;
My reading of ata_host_alloc() is that it allocates a refcounted 'struct
ata_host', but due to how we handle the failure of the following
devm_kzalloc(), we will never invoke ata_host_release() because the ref
won't be dropped anywhere.
So yes, the patch looks correct in the context shown by the patch
itself, but once we look at the entire function I think it's incorrect
(or, at least, I would expect more reasoning beyond "the static checker
told us so" around why it might be correct in the patch message itself
if I'm wrong).
--
Thanks,
Sasha
