| Message ID | c7d4a3ee8c615f5f6f468c0040fdb0e8864152ba.1784030076.git.ukleinek@kernel.org |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | pwm: tegra: Cleanups and .get_state() | expand |
On Tuesday, July 14, 2026 9:02 PM Uwe Kleine-König wrote: > It's unlikely but not impossible that of_device_get_match_data() returns > NULL. Handle this case instead of triggering a NULL pointer exception. > > Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org> > --- > drivers/pwm/pwm-tegra.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c > index 5cdbe120ba2d..53743f83869a 100644 > --- a/drivers/pwm/pwm-tegra.c > +++ b/drivers/pwm/pwm-tegra.c > @@ -322,6 +322,13 @@ static int tegra_pwm_probe(struct platform_device *pdev) > int ret; > > soc = of_device_get_match_data(&pdev->dev); > + if (!soc) Very subjective, but my preference is to have curly braces whenever the if block is more than one line, for clarity. > + /* > + * This can only happen if pdev was matched via pdev->name > + * (which should not happen today) or in combination with a > + * driver override. > + */ I feel like driver_override falls in the realm of 'root can mess with the system as they feel like but if they don't know what they're doing they get to keep the pieces'. So adding a check in every driver, or in practice having a random mix of drivers with and without the check, doesn't seem necessary to me. If we actually want to check for this condition, could it be done centrally instead? I.e. don't call probe if there's no match data and the driver's match table implies it requires it. > + return dev_err_probe(dev, -ENODEV, "Unsupported device\n"); 'dev' is not defined (yet). Thank you Mikko > > chip = devm_pwmchip_alloc(&pdev->dev, soc->num_channels, sizeof(*pc)); > if (IS_ERR(chip)) > -- > 2.55.0.11.g153666a7d9bb > >
Hello, On Wed, Jul 15, 2026 at 01:11:00PM +0900, Mikko Perttunen wrote: > On Tuesday, July 14, 2026 9:02 PM Uwe Kleine-König wrote: > > It's unlikely but not impossible that of_device_get_match_data() returns > > NULL. Handle this case instead of triggering a NULL pointer exception. > > > > Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org> > > --- > > drivers/pwm/pwm-tegra.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c > > index 5cdbe120ba2d..53743f83869a 100644 > > --- a/drivers/pwm/pwm-tegra.c > > +++ b/drivers/pwm/pwm-tegra.c > > @@ -322,6 +322,13 @@ static int tegra_pwm_probe(struct platform_device *pdev) > > int ret; > > > > soc = of_device_get_match_data(&pdev->dev); > > + if (!soc) > > Very subjective, but my preference is to have curly braces whenever the > if block is more than one line, for clarity. If you read Documentation/process/coding-style.rst by the letter, this case shouldn't have braces, but I agree that adding braces here is clearer (and that coding-style.rst shouldn't be read by the letter). > > + /* > > + * This can only happen if pdev was matched via pdev->name > > + * (which should not happen today) or in combination with a > > + * driver override. > > + */ > > I feel like driver_override falls in the realm of 'root can mess with > the system as they feel like but if they don't know what they're doing > they get to keep the pieces'. IMHO even root should not be able to trigger a NULL pointer exception. Not sure there is a general agreed on policy about that though. > So adding a check in every driver, or > in practice having a random mix of drivers with and without the check, > doesn't seem necessary to me. > > If we actually want to check for this condition, could it be done > centrally instead? I.e. don't call probe if there's no match data and > the driver's match table implies it requires it. So you'd want to check the device-id table before calling .probe() and if all entries have a non-zero .driver_data don't honor the override? Hmm, maybe something to discuss at the "Driver Core" microconference (at LPC 2026 in Prague), but my spontanous reaction is that this is a heuristic only and might prevent valid use-cases. > > + return dev_err_probe(dev, -ENODEV, "Unsupported device\n"); > > 'dev' is not defined (yet). Ooops, that leaked in as I reordered the patches to have the fixes first. Thanks for noticing. Best regards Uwe
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c index 5cdbe120ba2d..53743f83869a 100644 --- a/drivers/pwm/pwm-tegra.c +++ b/drivers/pwm/pwm-tegra.c @@ -322,6 +322,13 @@ static int tegra_pwm_probe(struct platform_device *pdev) int ret; soc = of_device_get_match_data(&pdev->dev); + if (!soc) + /* + * This can only happen if pdev was matched via pdev->name + * (which should not happen today) or in combination with a + * driver override. + */ + return dev_err_probe(dev, -ENODEV, "Unsupported device\n"); chip = devm_pwmchip_alloc(&pdev->dev, soc->num_channels, sizeof(*pc)); if (IS_ERR(chip))
It's unlikely but not impossible that of_device_get_match_data() returns NULL. Handle this case instead of triggering a NULL pointer exception. Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org> --- drivers/pwm/pwm-tegra.c | 7 +++++++ 1 file changed, 7 insertions(+)