[RFC,v4,7/8] pwm: Add dummy pwmchip for orphan pwms

Message ID 20171017101624.12506-8-jeffy.chen@rock-chips.com
State New
Headers show
Series
  • rockchip: kevin: Enable edp display
Related show

Commit Message

JeffyChen Oct. 17, 2017, 10:16 a.m.
When the pwm driver is unbound while the pwm is still requested, the
pwm core would not actually remove the pwmchip(return -EBUSY instead).

So it would hold some references to the invalid resources(e.g. pwmchip).

And the customer who requested the pwm would have those references too,
and may crash the kernel when trying to access them later.

Add a dummy pwmchip, and assign orphan pwms to it to avoid that.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v4:
Fix compile warning.

Changes in v3:
Assign orphan pwms to dummy pwmchip instead of adding device link in the
customer driver.

Changes in v2: None

 drivers/pwm/core.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 6 deletions(-)

Comments

Thierry Reding Oct. 17, 2017, 12:40 p.m. | #1
On Tue, Oct 17, 2017 at 06:16:23PM +0800, Jeffy Chen wrote:
> When the pwm driver is unbound while the pwm is still requested, the
> pwm core would not actually remove the pwmchip(return -EBUSY instead).
> 
> So it would hold some references to the invalid resources(e.g. pwmchip).
> 
> And the customer who requested the pwm would have those references too,
> and may crash the kernel when trying to access them later.
> 
> Add a dummy pwmchip, and assign orphan pwms to it to avoid that.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v4:
> Fix compile warning.
> 
> Changes in v3:
> Assign orphan pwms to dummy pwmchip instead of adding device link in the
> customer driver.

What happened to this? Device links were specifically designed to avoid
situations like these.

A dummy PWM chip doesn't seem like the right solution to this.

Thierry
Brian Norris Oct. 17, 2017, 5:04 p.m. | #2
Hi,

On Tue, Oct 17, 2017 at 02:40:31PM +0200, Thierry Reding wrote:
> On Tue, Oct 17, 2017 at 06:16:23PM +0800, Jeffy Chen wrote:
> > When the pwm driver is unbound while the pwm is still requested, the
> > pwm core would not actually remove the pwmchip(return -EBUSY instead).
> > 
> > So it would hold some references to the invalid resources(e.g. pwmchip).
> > 
> > And the customer who requested the pwm would have those references too,
> > and may crash the kernel when trying to access them later.
> > 
> > Add a dummy pwmchip, and assign orphan pwms to it to avoid that.
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> > 
> > Changes in v4:
> > Fix compile warning.
> > 
> > Changes in v3:
> > Assign orphan pwms to dummy pwmchip instead of adding device link in the
> > customer driver.
> 
> What happened to this? Device links were specifically designed to avoid
> situations like these.

I think Jeffy came up with this as an odd response to my suggestion on
v2 that we could just handle the device links in the PWM core. I don't
fully understand why the complete change in direction...

BTW, since you seem to have an opinion about device links: is it
expected that all consumer drivers will make explicit calls to
device_link_add()? I thought this should be avoided, if possible (e.g.,
this can be handled in pwm_get()).

> A dummy PWM chip doesn't seem like the right solution to this.

Agreed.

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
Dmitry Torokhov Oct. 17, 2017, 6:24 p.m. | #3
On Tue, Oct 17, 2017 at 10:04 AM, Brian Norris <briannorris@chromium.org> wrote:
> Hi,
>
> On Tue, Oct 17, 2017 at 02:40:31PM +0200, Thierry Reding wrote:
>> On Tue, Oct 17, 2017 at 06:16:23PM +0800, Jeffy Chen wrote:
>> > When the pwm driver is unbound while the pwm is still requested, the
>> > pwm core would not actually remove the pwmchip(return -EBUSY instead).
>> >
>> > So it would hold some references to the invalid resources(e.g. pwmchip).
>> >
>> > And the customer who requested the pwm would have those references too,
>> > and may crash the kernel when trying to access them later.
>> >
>> > Add a dummy pwmchip, and assign orphan pwms to it to avoid that.
>> >
>> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> > ---
>> >
>> > Changes in v4:
>> > Fix compile warning.
>> >
>> > Changes in v3:
>> > Assign orphan pwms to dummy pwmchip instead of adding device link in the
>> > customer driver.
>>
>> What happened to this? Device links were specifically designed to avoid
>> situations like these.
>
> I think Jeffy came up with this as an odd response to my suggestion on
> v2 that we could just handle the device links in the PWM core. I don't
> fully understand why the complete change in direction...
>
> BTW, since you seem to have an opinion about device links: is it
> expected that all consumer drivers will make explicit calls to
> device_link_add()? I thought this should be avoided, if possible (e.g.,
> this can be handled in pwm_get()).

Ideally we would not have this in core kernel API (pwm_get, gpiod_get,
regulator_get, etc) but retrieve it form the firmware (device tree,
ACPI) and use this data not only on suspend/resume but for probing as
well. *How exactly* can we do that is still not clear though, so maybe
we could plug the biggest holes by actually adding device links calls
to the main devm_<object>_get() users...

Thanks.
Mark Brown Oct. 17, 2017, 6:46 p.m. | #4
On Tue, Oct 17, 2017 at 11:24:24AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 17, 2017 at 10:04 AM, Brian Norris <briannorris@chromium.org> wrote:

> > BTW, since you seem to have an opinion about device links: is it
> > expected that all consumer drivers will make explicit calls to
> > device_link_add()? I thought this should be avoided, if possible (e.g.,
> > this can be handled in pwm_get()).

> Ideally we would not have this in core kernel API (pwm_get, gpiod_get,
> regulator_get, etc) but retrieve it form the firmware (device tree,
> ACPI) and use this data not only on suspend/resume but for probing as

Right, the major initial push here was for ordering of probes so doing
it in subsystems or drivers is too late.

> well. *How exactly* can we do that is still not clear though, so maybe
> we could plug the biggest holes by actually adding device links calls
> to the main devm_<object>_get() users...

I would expect we can get a long way in the DT by doing a pass over the
tree and adding links between device nodes in cases where phandle
references exist.  There is a potential issue with circular links which
I'm just going to handwave away right now but I'd expect that to help
otherwise.
Brian Norris Oct. 17, 2017, 6:53 p.m. | #5
On Tue, Oct 17, 2017 at 07:46:03PM +0100, Mark Brown wrote:
> On Tue, Oct 17, 2017 at 11:24:24AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 17, 2017 at 10:04 AM, Brian Norris <briannorris@chromium.org> wrote:
> 
> > > BTW, since you seem to have an opinion about device links: is it
> > > expected that all consumer drivers will make explicit calls to
> > > device_link_add()? I thought this should be avoided, if possible (e.g.,
> > > this can be handled in pwm_get()).
> 
> > Ideally we would not have this in core kernel API (pwm_get, gpiod_get,
> > regulator_get, etc) but retrieve it form the firmware (device tree,
> > ACPI) and use this data not only on suspend/resume but for probing as
> 
> Right, the major initial push here was for ordering of probes so doing
> it in subsystems or drivers is too late.
> 
> > well. *How exactly* can we do that is still not clear though, so maybe
> > we could plug the biggest holes by actually adding device links calls
> > to the main devm_<object>_get() users...
> 
> I would expect we can get a long way in the DT by doing a pass over the
> tree and adding links between device nodes in cases where phandle
> references exist.  There is a potential issue with circular links which
> I'm just going to handwave away right now but I'd expect that to help
> otherwise.

But I didn't think FDTs encoded type info. So you don't really know
whether a phandle is a phandle -- it's just an int (which happens to
have a corresponding property in some other node). Are we trusting our
DT bindings well enough to say that, for example, we know that in any
given device node, a property like 'pwms' must be a phandle to a PWM
provider? OK, maybe 'pwms' is a bad example (it's unlikely to get
reused, and it has a companion '#pwm-cells' property), but grepping the
DT bindings directory shows a ton of one-off properties that contain
phandles.

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
Mark Brown Oct. 17, 2017, 7:05 p.m. | #6
On Tue, Oct 17, 2017 at 11:53:01AM -0700, Brian Norris wrote:
> On Tue, Oct 17, 2017 at 07:46:03PM +0100, Mark Brown wrote:

> > I would expect we can get a long way in the DT by doing a pass over the
> > tree and adding links between device nodes in cases where phandle
> > references exist.  There is a potential issue with circular links which
> > I'm just going to handwave away right now but I'd expect that to help
> > otherwise.

> But I didn't think FDTs encoded type info. So you don't really know
> whether a phandle is a phandle -- it's just an int (which happens to
> have a corresponding property in some other node). Are we trusting our
> DT bindings well enough to say that, for example, we know that in any
> given device node, a property like 'pwms' must be a phandle to a PWM
> provider? OK, maybe 'pwms' is a bad example (it's unlikely to get
> reused, and it has a companion '#pwm-cells' property), but grepping the
> DT bindings directory shows a ton of one-off properties that contain
> phandles.

If we're going with the 90% thing we can probably get a long way with a
whitelist of properties, and we'll be able to take that a lot further
with the validatable schemas if they ever happen.
JeffyChen Oct. 18, 2017, 5:16 a.m. | #7
Hi guys,

On 10/18/2017 03:05 AM, Mark Brown wrote:
> On Tue, Oct 17, 2017 at 11:53:01AM -0700, Brian Norris wrote:
>> On Tue, Oct 17, 2017 at 07:46:03PM +0100, Mark Brown wrote:
>
>>> I would expect we can get a long way in the DT by doing a pass over the
>>> tree and adding links between device nodes in cases where phandle
>>> references exist.  There is a potential issue with circular links which
>>> I'm just going to handwave away right now but I'd expect that to help
>>> otherwise.
>
>> But I didn't think FDTs encoded type info. So you don't really know
>> whether a phandle is a phandle -- it's just an int (which happens to
>> have a corresponding property in some other node). Are we trusting our
>> DT bindings well enough to say that, for example, we know that in any
>> given device node, a property like 'pwms' must be a phandle to a PWM
>> provider? OK, maybe 'pwms' is a bad example (it's unlikely to get
>> reused, and it has a companion '#pwm-cells' property), but grepping the
>> DT bindings directory shows a ton of one-off properties that contain
>> phandles.
>
> If we're going with the 90% thing we can probably get a long way with a
> whitelist of properties, and we'll be able to take that a lot further
> with the validatable schemas if they ever happen.
>

so it looks like we are going to use device link in common code to fix 
this issue(and also other dependency issue), then i will drop this patch 
and the followed rockchip drm device link patch in next version :)


also the reason why i try to use dummy chip instead is that:

currently we have these devices: rockchip spi device(master) -> 
cros_ec_spi device(child) -> cros_ec_pwm(spi based pwm) -> pwm_bl

i added device link to cros_ec_pwm and pwm_bl, that works well for 
unbind/bind cros_ec_pwm device case, but there's a warning when i try to 
unbind cros_ec_spi:


static void device_links_purge(struct device *dev)
{
...
         list_for_each_entry_safe_reverse(link, ln, 
&dev->links.consumers, s_node) {
                 WARN_ON(link->status != DL_STATE_DORMANT &&
                         link->status != DL_STATE_NONE);        <--- hit 
warning here!
                 __device_link_del(link);
         }


but i checked again, that could due to the way spi core unregester 
children devices. maybe it need to call device_release_driver

--
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

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..0b6697bc8ab8 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -41,6 +41,21 @@  static LIST_HEAD(pwm_chips);
 static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
 static RADIX_TREE(pwm_tree, GFP_KERNEL);
 
+static int dummy_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   struct pwm_state *state)
+{
+	return -ENOTSUPP;
+}
+
+static const struct pwm_ops dummy_pwm_ops = {
+	.apply = dummy_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static struct device dummy_pwm_dev = {
+	.init_name = "dummy-pwm",
+};
+
 static struct pwm_device *pwm_to_device(unsigned int pwm)
 {
 	return radix_tree_lookup(&pwm_tree, pwm);
@@ -334,6 +349,33 @@  int pwmchip_add(struct pwm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
+static int pwmchip_convert_dummy(struct pwm_chip *chip)
+{
+	struct pwm_chip *dummy;
+	unsigned int i;
+
+	dummy = kzalloc(sizeof(*dummy), GFP_KERNEL);
+	if (!dummy)
+		return -ENOMEM;
+
+	dummy->dev = &dummy_pwm_dev;
+	dummy->ops = &dummy_pwm_ops;
+	dummy->npwm = chip->npwm;
+	dummy->pwms = chip->pwms;
+
+	INIT_LIST_HEAD(&dummy->list);
+	list_add(&dummy->list, &pwm_chips);
+
+	for (i = 0; i < dummy->npwm; i++) {
+		struct pwm_device *pwm = &dummy->pwms[i];
+
+		pwm->chip = dummy;
+		pwm->state.enabled = false;
+	}
+
+	return 0;
+}
+
 /**
  * pwmchip_remove() - remove a PWM chip
  * @chip: the PWM chip to remove
@@ -346,7 +388,7 @@  EXPORT_SYMBOL_GPL(pwmchip_add);
 int pwmchip_remove(struct pwm_chip *chip)
 {
 	unsigned int i;
-	int ret = 0;
+	int ret = 0, requested = 0;
 
 	pwmchip_sysfs_unexport_children(chip);
 
@@ -356,8 +398,12 @@  int pwmchip_remove(struct pwm_chip *chip)
 		struct pwm_device *pwm = &chip->pwms[i];
 
 		if (test_bit(PWMF_REQUESTED, &pwm->flags)) {
-			ret = -EBUSY;
-			goto out;
+			requested = 1;
+
+			if (pwm->chip->ops->free)
+				pwm->chip->ops->free(pwm->chip, pwm);
+
+			module_put(pwm->chip->ops->owner);
 		}
 	}
 
@@ -366,11 +412,13 @@  int pwmchip_remove(struct pwm_chip *chip)
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_remove(chip);
 
-	free_pwms(chip);
+	if (requested)
+		pwmchip_convert_dummy(chip);
+	else
+		free_pwms(chip);
 
 	pwmchip_sysfs_unexport(chip);
 
-out:
 	mutex_unlock(&pwm_lock);
 	return ret;
 }
@@ -855,6 +903,24 @@  struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL_GPL(pwm_get);
 
+static int pwmchip_remove_dummy(struct pwm_chip *dummy)
+{
+	unsigned int i;
+
+	for (i = 0; i < dummy->npwm; i++) {
+		struct pwm_device *pwm = &dummy->pwms[i];
+
+		if (test_bit(PWMF_REQUESTED, &pwm->flags))
+			return -EBUSY;
+	}
+
+	free_pwms(dummy);
+	list_del_init(&dummy->list);
+	kfree(dummy);
+
+	return 0;
+}
+
 /**
  * pwm_put() - release a PWM device
  * @pwm: PWM device
@@ -876,7 +942,10 @@  void pwm_put(struct pwm_device *pwm)
 
 	pwm->label = NULL;
 
-	module_put(pwm->chip->ops->owner);
+	if (pwm->chip->ops == &dummy_pwm_ops)
+		pwmchip_remove_dummy(pwm->chip);
+	else
+		module_put(pwm->chip->ops->owner);
 out:
 	mutex_unlock(&pwm_lock);
 }