linux-kernel - Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 4 Dec 2020 13:41:16 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
        Lee Jones <lee.jones@...aro.org>, kernel@...gutronix.de,
        Michael Walle <michael@...le.cc>
Subject: Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data
 in pwm callbacks

On Thu, Dec 03, 2020 at 09:41:42AM +0100, Uwe Kleine-König wrote:
> Currently .get_state() and .apply() use dev_get_drvdata() on the struct
> device related to the pwm chip. This only works after .probe() called
> platform_set_drvdata() which in this driver happens only after
> pwmchip_add() and so comes possibly too late.
> 
> Instead of setting the driver data earlier use the traditional
> container_of approach as this way the driver data is conceptually and
> computational nearer.
> 
> Fixes: 9db33d221efc ("pwm: Add support for sl28cpld PWM controller")
> Tested-by: Michael Walle <michael@...le.cc>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> ---
> Hello Linus,
> 
> Thierry (who usually sends PWM patches to you) didn't react to this
> patch sent to the pwm Mailinglist last week
> (https://lore.kernel.org/r/20201124212432.3117322-1-u.kleine-koenig@pengutronix.de)
> yet.
> 
> Given v5.10 isn't far away any more and I don't know when Thierry will
> take a look and act, I'm sending this directly to you. The affected
> driver was new in 5.10-rc1 and at least once the unpatched driver
> created an oops:
> 
> 	https://lavalab.kontron.com/scheduler/job/108#L950
> 
> Michael Walle who tested this patch is the original author of the
> driver. IMHO it would be good to have this fixed before 5.10.
> 
> If you prefer a pull request, I can setup something (but I don't have
> access to Thierry's tree, so it will be for a repository that's new to
> you).
> 
> Best regards
> Uwe
> 
>  drivers/pwm/pwm-sl28cpld.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

I thought I had seen you discuss this with Lee and gotten the impression
that you were going to respin this to move the platform_set_drvdata() to
an earlier point, which I think is the more correct approach.

container_of() isn't exactly wrong, but it's really just papering over
the fact that platform_set_drvdata() is in the wrong place, so I'd
prefer a patch that does that instead.

Now, I can no longer find a link to the discussion that I recall, so it
was either on IRC (where I don't have any logs) or I'm loosing my mind.

I'll prepare a patch that moves platform_set_drvdata() for Michael to
test. If that works I'll send a PR with fixes to Linus early next week.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)