| Message ID | 20171016100640.26575-3-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Oct 16, 2017 at 06:06:37PM +0800, Jeffy Chen wrote: > When the pwm driver is unbound, the pwm_bl driver would still hold a > reference to that pwm, and crash the kernel later(if someone trying > to access that invalid pwm). This is not the primary problem you're trying to address though, is it? This is mostly supposed to handle PM, not device removal (though it seems to do some of both). But for the removal/unbind case, I wondered why the existing PWM "requested" status [1] didn't catch the stated problem above, but then I noticed...the driver core doesn't care if the driver remove() callback fails. So pwmchip_remove() an return -EBUSY all it wants -- the device core is still going to unbind you (and free all your devm_*'s). This seems kinda bad. > Add a device link to avoid this. This is going to be a *lot* of churn throughout the tree, if we expect all resource consumers to do this. I think we'd want some kind of agreement from the PM maintainers and (larger) subsystem owners before going down this route... And in the PWM case, pwm_get() already has the device pointer. Why can't we just instrument it instead? Brian P.S. A little off-topic, but this enum is wrong, for use with test_bit(): enum { PWMF_REQUESTED = 1 << 0, PWMF_EXPORTED = 1 << 1, }; test_bit() and friends take a bit number, not a bit mask. Fortunately it doesn't matter much, since the bitmask is just not very dense this way. But it's misleading and will cause problems if we get a lot new flags. > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > Changes in v2: None > > drivers/video/backlight/pwm_bl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 9bd17682655a..a76f147a26e7 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -328,6 +328,8 @@ static int pwm_backlight_probe(struct platform_device *pdev) > goto err_alloc; > } > > + device_link_add(&pdev->dev, pb->pwm->chip->dev, DL_FLAG_AUTOREMOVE); > + > dev_dbg(&pdev->dev, "got pwm for backlight\n"); > > /* > -- > 2.11.0 > > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, On 10/17/2017 07:57 AM, Brian Norris wrote: > This is going to be a*lot* of churn throughout the tree, if we expect > all resource consumers to do this. I think we'd want some kind of > agreement from the PM maintainers and (larger) subsystem owners before > going down this route... > > And in the PWM case, pwm_get() already has the device pointer. Why can't > we just instrument it instead? according to pwm_bl driver, we may need to take care of pwm_request() too: pb->pwm = devm_pwm_get(&pdev->dev, NULL); if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !node) { dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n"); pb->legacy = true; pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); } and maybe also *of_pwm_get... maybe we can add a dummy pwm chip for those orphan pwms? > > Brian -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Oct 17, 2017 at 04:13:55PM +0800, Jeffy Chen wrote: > On 10/17/2017 07:57 AM, Brian Norris wrote: > >This is going to be a*lot* of churn throughout the tree, if we expect > >all resource consumers to do this. I think we'd want some kind of > >agreement from the PM maintainers and (larger) subsystem owners before > >going down this route... > > > >And in the PWM case, pwm_get() already has the device pointer. Why can't > >we just instrument it instead? > > according to pwm_bl driver, we may need to take care of pwm_request() too: That's a legacy API. I wouldn't spend any time on improving it. In fact, the only other 2 users are: (a) drivers/input/misc/max8997_haptic.c: abandoned; nobody provides pdata for that driver, so pwm_request() can never be called... (b) arch/arm/mach-s3c24xx/mach-rx1950.c: can be easily converted to the lookup table approach (pwm_add_table() + pwm_get()) if needed > pb->pwm = devm_pwm_get(&pdev->dev, NULL); > if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && > !node) { > dev_err(&pdev->dev, "unable to request PWM, trying > legacy API\n"); > pb->legacy = true; > pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); > } > > and maybe also *of_pwm_get... > > maybe we can add a dummy pwm chip for those orphan pwms? What? That seems like a very silly idea. And judging by Thierry's response to your v4, he doesn't understand it either. All I was suggesting was that you should try to add the device links in the fewest places possible. Because if you require all consumers to add extra boilerplate to resolve some strange corner cases, those corner cases will likely go unsolved in many cases. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 9bd17682655a..a76f147a26e7 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -328,6 +328,8 @@ static int pwm_backlight_probe(struct platform_device *pdev) goto err_alloc; } + device_link_add(&pdev->dev, pb->pwm->chip->dev, DL_FLAG_AUTOREMOVE); + dev_dbg(&pdev->dev, "got pwm for backlight\n"); /*
When the pwm driver is unbound, the pwm_bl driver would still hold a reference to that pwm, and crash the kernel later(if someone trying to access that invalid pwm). Add a device link to avoid this. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- Changes in v2: None drivers/video/backlight/pwm_bl.c | 2 ++ 1 file changed, 2 insertions(+)