Message ID | 20211227094526.698714-16-u.kleine-koenig@pengutronix.de (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | [v2,01/23] counter: Use container_of instead of drvdata to track counter_device | expand |
On 12/27/21 10:45 AM, Uwe Kleine-König wrote: > [...] > > - return devm_counter_register(dev, &priv->counter); > + err = devm_counter_add(dev, counter); > + if (err < 0) > + return dev_err_probe(dev, err, "Failed to add counter\n"); I wonder if we should put that dev_err_probe into the devm_counter_add since every driver wants to have it anyway. > + > + return 0; > } > > static struct isa_driver quad8_driver = {
On Mon, Dec 27, 2021 at 01:19:25PM +0100, Lars-Peter Clausen wrote: > On 12/27/21 10:45 AM, Uwe Kleine-König wrote: > > [...] > > - return devm_counter_register(dev, &priv->counter); > > + err = devm_counter_add(dev, counter); > > + if (err < 0) > > + return dev_err_probe(dev, err, "Failed to add counter\n"); > I wonder if we should put that dev_err_probe into the devm_counter_add since > every driver wants to have it anyway. Personally I'm not a big fan of API functions that emit an error message. Usually the consumer knows best if a certain failing function call is worth an error message. Look at platform_get_irq() that some time ago got a dev_err call. The result was to introduce a silent variant (platform_get_irq_optional()) because emitting an error message isn't the right thing in all cases. Also the API function probably won't call device_set_deferred_probe_reason() and so the consumer has to care for that anyhow (or not which makes -EPROBE_DEFER problems harder to debug). Furthermore the consumer can emit in some cases a better error message than the core. (Well, this doesn't apply here, but in the example of platform_get_irq() the driver knows the irq's purpose and can call it "TX irq" in the error message which the irq core cannot.) Best regards Uwe
On Mon, 27 Dec 2021 10:45:18 +0100 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > This fixes device lifetime issues where it was possible to free a live > struct device. > > Fixes: f1d8a071d45b ("counter: 104-quad-8: Add Generic Counter interface support") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/counter/104-quad-8.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c > index 6e5286cd1d4e..4315b14f239e 100644 > --- a/drivers/counter/104-quad-8.c > +++ b/drivers/counter/104-quad-8.c > @@ -52,7 +52,6 @@ MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers"); > */ > struct quad8 { > spinlock_t lock; > - struct counter_device counter; > unsigned int fck_prescaler[QUAD8_NUM_COUNTERS]; > unsigned int preset[QUAD8_NUM_COUNTERS]; > unsigned int count_mode[QUAD8_NUM_COUNTERS]; > @@ -1127,6 +1126,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private) > > static int quad8_probe(struct device *dev, unsigned int id) > { > + struct counter_device *counter; > struct quad8 *priv; > int i, j; > unsigned int base_offset; > @@ -1138,19 +1138,19 @@ static int quad8_probe(struct device *dev, unsigned int id) > return -EBUSY; > } > > - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > - if (!priv) > + counter = devm_counter_alloc(dev, sizeof(*priv)); > + if (!counter) > return -ENOMEM; > + priv = counter_priv(counter); > > /* Initialize Counter device and driver data */ > - priv->counter.name = dev_name(dev); > - priv->counter.parent = dev; > - priv->counter.ops = &quad8_ops; > - priv->counter.counts = quad8_counts; > - priv->counter.num_counts = ARRAY_SIZE(quad8_counts); > - priv->counter.signals = quad8_signals; > - priv->counter.num_signals = ARRAY_SIZE(quad8_signals); > - priv->counter.priv = priv; > + counter->name = dev_name(dev); > + counter->parent = dev; > + counter->ops = &quad8_ops; > + counter->counts = quad8_counts; > + counter->num_counts = ARRAY_SIZE(quad8_counts); > + counter->signals = quad8_signals; > + counter->num_signals = ARRAY_SIZE(quad8_signals); > priv->base = base[id]; > > spin_lock_init(&priv->lock); > @@ -1192,11 +1192,15 @@ static int quad8_probe(struct device *dev, unsigned int id) > outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP); > > err = devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED, > - priv->counter.name, priv); > + counter->name, priv); > if (err) > return err; > > - return devm_counter_register(dev, &priv->counter); > + err = devm_counter_add(dev, counter); > + if (err < 0) > + return dev_err_probe(dev, err, "Failed to add counter\n"); > + > + return 0; > } > > static struct isa_driver quad8_driver = {
On Mon, Dec 27, 2021 at 10:45:18AM +0100, Uwe Kleine-König wrote: > This fixes device lifetime issues where it was possible to free a live > struct device. > > Fixes: f1d8a071d45b ("counter: 104-quad-8: Add Generic Counter interface support") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> > --- > drivers/counter/104-quad-8.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c > index 6e5286cd1d4e..4315b14f239e 100644 > --- a/drivers/counter/104-quad-8.c > +++ b/drivers/counter/104-quad-8.c > @@ -52,7 +52,6 @@ MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers"); > */ > struct quad8 { > spinlock_t lock; > - struct counter_device counter; > unsigned int fck_prescaler[QUAD8_NUM_COUNTERS]; > unsigned int preset[QUAD8_NUM_COUNTERS]; > unsigned int count_mode[QUAD8_NUM_COUNTERS]; > @@ -1127,6 +1126,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private) > > static int quad8_probe(struct device *dev, unsigned int id) > { > + struct counter_device *counter; > struct quad8 *priv; > int i, j; > unsigned int base_offset; > @@ -1138,19 +1138,19 @@ static int quad8_probe(struct device *dev, unsigned int id) > return -EBUSY; > } > > - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > - if (!priv) > + counter = devm_counter_alloc(dev, sizeof(*priv)); > + if (!counter) > return -ENOMEM; > + priv = counter_priv(counter); > > /* Initialize Counter device and driver data */ > - priv->counter.name = dev_name(dev); > - priv->counter.parent = dev; > - priv->counter.ops = &quad8_ops; > - priv->counter.counts = quad8_counts; > - priv->counter.num_counts = ARRAY_SIZE(quad8_counts); > - priv->counter.signals = quad8_signals; > - priv->counter.num_signals = ARRAY_SIZE(quad8_signals); > - priv->counter.priv = priv; > + counter->name = dev_name(dev); > + counter->parent = dev; > + counter->ops = &quad8_ops; > + counter->counts = quad8_counts; > + counter->num_counts = ARRAY_SIZE(quad8_counts); > + counter->signals = quad8_signals; > + counter->num_signals = ARRAY_SIZE(quad8_signals); > priv->base = base[id]; > > spin_lock_init(&priv->lock); > @@ -1192,11 +1192,15 @@ static int quad8_probe(struct device *dev, unsigned int id) > outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP); > > err = devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED, > - priv->counter.name, priv); > + counter->name, priv); > if (err) > return err; > > - return devm_counter_register(dev, &priv->counter); > + err = devm_counter_add(dev, counter); > + if (err < 0) > + return dev_err_probe(dev, err, "Failed to add counter\n"); > + > + return 0; > } > > static struct isa_driver quad8_driver = { > -- > 2.33.0 >
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c index 6e5286cd1d4e..4315b14f239e 100644 --- a/drivers/counter/104-quad-8.c +++ b/drivers/counter/104-quad-8.c @@ -52,7 +52,6 @@ MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers"); */ struct quad8 { spinlock_t lock; - struct counter_device counter; unsigned int fck_prescaler[QUAD8_NUM_COUNTERS]; unsigned int preset[QUAD8_NUM_COUNTERS]; unsigned int count_mode[QUAD8_NUM_COUNTERS]; @@ -1127,6 +1126,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private) static int quad8_probe(struct device *dev, unsigned int id) { + struct counter_device *counter; struct quad8 *priv; int i, j; unsigned int base_offset; @@ -1138,19 +1138,19 @@ static int quad8_probe(struct device *dev, unsigned int id) return -EBUSY; } - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); - if (!priv) + counter = devm_counter_alloc(dev, sizeof(*priv)); + if (!counter) return -ENOMEM; + priv = counter_priv(counter); /* Initialize Counter device and driver data */ - priv->counter.name = dev_name(dev); - priv->counter.parent = dev; - priv->counter.ops = &quad8_ops; - priv->counter.counts = quad8_counts; - priv->counter.num_counts = ARRAY_SIZE(quad8_counts); - priv->counter.signals = quad8_signals; - priv->counter.num_signals = ARRAY_SIZE(quad8_signals); - priv->counter.priv = priv; + counter->name = dev_name(dev); + counter->parent = dev; + counter->ops = &quad8_ops; + counter->counts = quad8_counts; + counter->num_counts = ARRAY_SIZE(quad8_counts); + counter->signals = quad8_signals; + counter->num_signals = ARRAY_SIZE(quad8_signals); priv->base = base[id]; spin_lock_init(&priv->lock); @@ -1192,11 +1192,15 @@ static int quad8_probe(struct device *dev, unsigned int id) outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP); err = devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED, - priv->counter.name, priv); + counter->name, priv); if (err) return err; - return devm_counter_register(dev, &priv->counter); + err = devm_counter_add(dev, counter); + if (err < 0) + return dev_err_probe(dev, err, "Failed to add counter\n"); + + return 0; } static struct isa_driver quad8_driver = {
This fixes device lifetime issues where it was possible to free a live struct device. Fixes: f1d8a071d45b ("counter: 104-quad-8: Add Generic Counter interface support") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/counter/104-quad-8.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)