Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register
On Wed, Oct 16, 2019 at 6:14 PM Russell King - ARM Linux admin wrote: > > On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote: > > From: Dariusz Marcinkiewicz > > > > Use the new cec_notifier_conn_(un)register() functions to > > (un)register the notifier for the HDMI connector. > > > > Signed-off-by: Dariusz Marcinkiewicz > > Signed-off-by: Hans Verkuil > > Please explain in detail what this mutex actually achieves. > > So, all in all, I don't see what this lock is doing, and I think it > should be removed. > > If it's necessary for a future change (which may or may not be merged) > then the lock should be part of that future change, because the change > proposed by this patch certainly does not need it. > Yes, with the change being split into 2 patches like that, the mutex is not needed here. Thank you.
Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register
On 10/17/19 9:03 AM, Hans Verkuil wrote:
> On 10/16/19 6:14 PM, Russell King - ARM Linux admin wrote:
>> On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote:
>>> From: Dariusz Marcinkiewicz
>>>
>>> Use the new cec_notifier_conn_(un)register() functions to
>>> (un)register the notifier for the HDMI connector.
>>>
>>> Signed-off-by: Dariusz Marcinkiewicz
>>> Signed-off-by: Hans Verkuil
>>
>> Please explain in detail what this mutex actually achieves.
>
> Dariusz, since you're the author, can you reply to Russell?
>
> If this is going to be a delaying factor, then I'll post a new version
> without the mutex that just replaces the cec_notifier API.
I decided to post a v9, moving the mutex to the second patch, which should
make the first patch acceptable to everyone for v5.5.
Regards,
Hans
>
> Regards,
>
> Hans
>
>>
>>> ---
>>> drivers/gpu/drm/i2c/tda998x_drv.c | 21 -
>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
>>> b/drivers/gpu/drm/i2c/tda998x_drv.c
>>> index 84c6d4c91c65..8262b44b776e 100644
>>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>>> @@ -82,6 +82,9 @@ struct tda998x_priv {
>>> u8 audio_port_enable[AUDIO_ROUTE_NUM];
>>> struct tda9950_glue cec_glue;
>>> struct gpio_desc *calib;
>>> +
>>> + /* protect cec_notify */
>>> + struct mutex cec_notify_mutex;
>>> struct cec_notifier *cec_notify;
>>> };
>>>
>>> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void
>>> *data)
>>> tda998x_edid_delay_start(priv);
>>> } else {
>>> schedule_work(&priv->detect_work);
>>> - cec_notifier_set_phys_addr(priv->cec_notify,
>>> - CEC_PHYS_ADDR_INVALID);
>>> +
>>> + mutex_lock(&priv->cec_notify_mutex);
>>> + cec_notifier_phys_addr_invalidate(
>>> + priv->cec_notify);
>>> + mutex_unlock(&priv->cec_notify_mutex);
>>> }
>>>
>>> handled = true;
>>> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev)
>>>
>>> i2c_unregister_device(priv->cec);
>>>
>>> - if (priv->cec_notify)
>>> - cec_notifier_put(priv->cec_notify);
>>> + mutex_lock(&priv->cec_notify_mutex);
>>> + cec_notifier_conn_unregister(priv->cec_notify);
>>> + priv->cec_notify = NULL;
>>> + mutex_unlock(&priv->cec_notify_mutex);
>>
>> By the time we get here:
>>
>> 1) The interrupt has been freed (which is a synchronous operation)
>>tda998x_irq_thread() can't be called and can't be running, and
>>therefore cec_notifier_phys_addr_invalidate() also can't be called
>>or be running.
>> 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site;
>>if there's any case that _might_ possibly conflict, it is that one.
>> 3) tda998x_destroy() and tda998x_create() can't be called concurrently
>>in any case; the driver model guarantees that ->probe and ->remove
>>for the same device are serialised.
>>
>>> }
>>>
>>> static int tda998x_create(struct device *dev)
>>> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev)
>>> mutex_init(&priv->mutex); /* protect the page access */
>>> mutex_init(&priv->audio_mutex); /* protect access from audio thread */
>>> mutex_init(&priv->edid_mutex);
>>> + mutex_init(&priv->cec_notify_mutex);
>>> INIT_LIST_HEAD(&priv->bridge.list);
>>> init_waitqueue_head(&priv->edid_delay_waitq);
>>> timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
>>> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev)
>>> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
>>> }
>>>
>>> - priv->cec_notify = cec_notifier_get(dev);
>>> + mutex_lock(&priv->cec_notify_mutex);
>>> + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
>>> + mutex_unlock(&priv->cec_notify_mutex);
>>
>> and:
>>
>> 4) priv->cec_notify will be NULL here until such time that
>>cec_notifier_conn_register() has returned. If the mutex is trying
>>to protect something, it's very unclear what it is.
>>
>> Trying to guess what it's protecting against:
>>
>> - Is it protecting against NULL priv->cec_notify? No, because it can
>> be NULL just before we take the lock.
>> - Is it protecting the internals of cec_notifier_conn_register()
>> against the other calls - no, because priv->cec_notify will be NULL
>> until the function has finished.
>> - Is it protecting the write to priv->cec_notify? Maybe, but that
>> doesn't need any protection - architectures are single-copy atomic,
>> which means that a pointer is either written or it is not written.
>> Therefore, it will either be NULL (the
Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register
On 10/16/19 6:14 PM, Russell King - ARM Linux admin wrote:
> On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote:
>> From: Dariusz Marcinkiewicz
>>
>> Use the new cec_notifier_conn_(un)register() functions to
>> (un)register the notifier for the HDMI connector.
>>
>> Signed-off-by: Dariusz Marcinkiewicz
>> Signed-off-by: Hans Verkuil
>
> Please explain in detail what this mutex actually achieves.
Dariusz, since you're the author, can you reply to Russell?
If this is going to be a delaying factor, then I'll post a new version
without the mutex that just replaces the cec_notifier API.
Regards,
Hans
>
>> ---
>> drivers/gpu/drm/i2c/tda998x_drv.c | 21 -
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
>> b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index 84c6d4c91c65..8262b44b776e 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -82,6 +82,9 @@ struct tda998x_priv {
>> u8 audio_port_enable[AUDIO_ROUTE_NUM];
>> struct tda9950_glue cec_glue;
>> struct gpio_desc *calib;
>> +
>> +/* protect cec_notify */
>> +struct mutex cec_notify_mutex;
>> struct cec_notifier *cec_notify;
>> };
>>
>> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void
>> *data)
>> tda998x_edid_delay_start(priv);
>> } else {
>> schedule_work(&priv->detect_work);
>> -cec_notifier_set_phys_addr(priv->cec_notify,
>> - CEC_PHYS_ADDR_INVALID);
>> +
>> +mutex_lock(&priv->cec_notify_mutex);
>> +cec_notifier_phys_addr_invalidate(
>> +priv->cec_notify);
>> +mutex_unlock(&priv->cec_notify_mutex);
>> }
>>
>> handled = true;
>> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev)
>>
>> i2c_unregister_device(priv->cec);
>>
>> -if (priv->cec_notify)
>> -cec_notifier_put(priv->cec_notify);
>> +mutex_lock(&priv->cec_notify_mutex);
>> +cec_notifier_conn_unregister(priv->cec_notify);
>> +priv->cec_notify = NULL;
>> +mutex_unlock(&priv->cec_notify_mutex);
>
> By the time we get here:
>
> 1) The interrupt has been freed (which is a synchronous operation)
>tda998x_irq_thread() can't be called and can't be running, and
>therefore cec_notifier_phys_addr_invalidate() also can't be called
>or be running.
> 2) You don't touch the cec_notifier_set_phys_addr_from_edid() site;
>if there's any case that _might_ possibly conflict, it is that one.
> 3) tda998x_destroy() and tda998x_create() can't be called concurrently
>in any case; the driver model guarantees that ->probe and ->remove
>for the same device are serialised.
>
>> }
>>
>> static int tda998x_create(struct device *dev)
>> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev)
>> mutex_init(&priv->mutex); /* protect the page access */
>> mutex_init(&priv->audio_mutex); /* protect access from audio thread */
>> mutex_init(&priv->edid_mutex);
>> +mutex_init(&priv->cec_notify_mutex);
>> INIT_LIST_HEAD(&priv->bridge.list);
>> init_waitqueue_head(&priv->edid_delay_waitq);
>> timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
>> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev)
>> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
>> }
>>
>> -priv->cec_notify = cec_notifier_get(dev);
>> +mutex_lock(&priv->cec_notify_mutex);
>> +priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
>> +mutex_unlock(&priv->cec_notify_mutex);
>
> and:
>
> 4) priv->cec_notify will be NULL here until such time that
>cec_notifier_conn_register() has returned. If the mutex is trying
>to protect something, it's very unclear what it is.
>
> Trying to guess what it's protecting against:
>
> - Is it protecting against NULL priv->cec_notify? No, because it can
> be NULL just before we take the lock.
> - Is it protecting the internals of cec_notifier_conn_register()
> against the other calls - no, because priv->cec_notify will be NULL
> until the function has finished.
> - Is it protecting the write to priv->cec_notify? Maybe, but that
> doesn't need any protection - architectures are single-copy atomic,
> which means that a pointer is either written or it is not written.
> Therefore, it will either be NULL (the state before the call is made)
> or it will be set correctly (after the call has completed.)
>
> So, all in all, I don't see what this lock is doing, and I think it
> should be removed.
>
> If it's necessary for a future change (which may or may not be merged)
> then the
Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register
On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote:
> From: Dariusz Marcinkiewicz
>
> Use the new cec_notifier_conn_(un)register() functions to
> (un)register the notifier for the HDMI connector.
>
> Signed-off-by: Dariusz Marcinkiewicz
> Signed-off-by: Hans Verkuil
Please explain in detail what this mutex actually achieves.
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 21 -
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 84c6d4c91c65..8262b44b776e 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -82,6 +82,9 @@ struct tda998x_priv {
> u8 audio_port_enable[AUDIO_ROUTE_NUM];
> struct tda9950_glue cec_glue;
> struct gpio_desc *calib;
> +
> + /* protect cec_notify */
> + struct mutex cec_notify_mutex;
> struct cec_notifier *cec_notify;
> };
>
> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void
> *data)
> tda998x_edid_delay_start(priv);
> } else {
> schedule_work(&priv->detect_work);
> - cec_notifier_set_phys_addr(priv->cec_notify,
> -CEC_PHYS_ADDR_INVALID);
> +
> + mutex_lock(&priv->cec_notify_mutex);
> + cec_notifier_phys_addr_invalidate(
> + priv->cec_notify);
> + mutex_unlock(&priv->cec_notify_mutex);
> }
>
> handled = true;
> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev)
>
> i2c_unregister_device(priv->cec);
>
> - if (priv->cec_notify)
> - cec_notifier_put(priv->cec_notify);
> + mutex_lock(&priv->cec_notify_mutex);
> + cec_notifier_conn_unregister(priv->cec_notify);
> + priv->cec_notify = NULL;
> + mutex_unlock(&priv->cec_notify_mutex);
By the time we get here:
1) The interrupt has been freed (which is a synchronous operation)
tda998x_irq_thread() can't be called and can't be running, and
therefore cec_notifier_phys_addr_invalidate() also can't be called
or be running.
2) You don't touch the cec_notifier_set_phys_addr_from_edid() site;
if there's any case that _might_ possibly conflict, it is that one.
3) tda998x_destroy() and tda998x_create() can't be called concurrently
in any case; the driver model guarantees that ->probe and ->remove
for the same device are serialised.
> }
>
> static int tda998x_create(struct device *dev)
> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev)
> mutex_init(&priv->mutex); /* protect the page access */
> mutex_init(&priv->audio_mutex); /* protect access from audio thread */
> mutex_init(&priv->edid_mutex);
> + mutex_init(&priv->cec_notify_mutex);
> INIT_LIST_HEAD(&priv->bridge.list);
> init_waitqueue_head(&priv->edid_delay_waitq);
> timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev)
> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
> }
>
> - priv->cec_notify = cec_notifier_get(dev);
> + mutex_lock(&priv->cec_notify_mutex);
> + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
> + mutex_unlock(&priv->cec_notify_mutex);
and:
4) priv->cec_notify will be NULL here until such time that
cec_notifier_conn_register() has returned. If the mutex is trying
to protect something, it's very unclear what it is.
Trying to guess what it's protecting against:
- Is it protecting against NULL priv->cec_notify? No, because it can
be NULL just before we take the lock.
- Is it protecting the internals of cec_notifier_conn_register()
against the other calls - no, because priv->cec_notify will be NULL
until the function has finished.
- Is it protecting the write to priv->cec_notify? Maybe, but that
doesn't need any protection - architectures are single-copy atomic,
which means that a pointer is either written or it is not written.
Therefore, it will either be NULL (the state before the call is made)
or it will be set correctly (after the call has completed.)
So, all in all, I don't see what this lock is doing, and I think it
should be removed.
If it's necessary for a future change (which may or may not be merged)
then the lock should be part of that future change, because the change
proposed by this patch certainly does not need it.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
[PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register
From: Dariusz Marcinkiewicz
Use the new cec_notifier_conn_(un)register() functions to
(un)register the notifier for the HDMI connector.
Signed-off-by: Dariusz Marcinkiewicz
Signed-off-by: Hans Verkuil
---
drivers/gpu/drm/i2c/tda998x_drv.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 84c6d4c91c65..8262b44b776e 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -82,6 +82,9 @@ struct tda998x_priv {
u8 audio_port_enable[AUDIO_ROUTE_NUM];
struct tda9950_glue cec_glue;
struct gpio_desc *calib;
+
+ /* protect cec_notify */
+ struct mutex cec_notify_mutex;
struct cec_notifier *cec_notify;
};
@@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
tda998x_edid_delay_start(priv);
} else {
schedule_work(&priv->detect_work);
- cec_notifier_set_phys_addr(priv->cec_notify,
- CEC_PHYS_ADDR_INVALID);
+
+ mutex_lock(&priv->cec_notify_mutex);
+ cec_notifier_phys_addr_invalidate(
+ priv->cec_notify);
+ mutex_unlock(&priv->cec_notify_mutex);
}
handled = true;
@@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev)
i2c_unregister_device(priv->cec);
- if (priv->cec_notify)
- cec_notifier_put(priv->cec_notify);
+ mutex_lock(&priv->cec_notify_mutex);
+ cec_notifier_conn_unregister(priv->cec_notify);
+ priv->cec_notify = NULL;
+ mutex_unlock(&priv->cec_notify_mutex);
}
static int tda998x_create(struct device *dev)
@@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev)
mutex_init(&priv->mutex); /* protect the page access */
mutex_init(&priv->audio_mutex); /* protect access from audio thread */
mutex_init(&priv->edid_mutex);
+ mutex_init(&priv->cec_notify_mutex);
INIT_LIST_HEAD(&priv->bridge.list);
init_waitqueue_head(&priv->edid_delay_waitq);
timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
@@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev)
cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
}
- priv->cec_notify = cec_notifier_get(dev);
+ mutex_lock(&priv->cec_notify_mutex);
+ priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
+ mutex_unlock(&priv->cec_notify_mutex);
if (!priv->cec_notify) {
ret = -ENOMEM;
goto fail;
--
2.23.0
