[RESEND,v2,2/5] backlight: pwm_bl: Add device link for pwm_bl and pwm

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(+)

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-pwm" 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-pwm" 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-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html