On 6/25/19 3:48 PM, Dariusz Marcinkiewicz wrote:
> Hello.
>
> Some small comments/questions.
>
> On Mon, Jun 24, 2019 at 6:03 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>>
> ...
>> @@ -22,9 +22,11 @@ struct cec_notifier {
>> struct list_head head;
>> struct kref kref;
>> struct device *hdmi_dev;
>> + struct cec_connector_info conn_info;
>> const char *conn_name;
>> struct cec_adapter *cec_adap;
>> void (*callback)(struct cec_adapter *adap, u16 pa);
>> + bool called_cec_notifier_register;
> If I read his correctly callback is set only by cec_notifier_register
> (and not by the cec_notifier_cec_adap_register) so maybe that boolean
> is not needed and just checking if the callback is set is enough to
> tell those 2 cases apart?
True. I'll change this.
>
> ...
>> +struct cec_notifier *
>> +cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *conn_name,
>> + struct cec_adapter *adap)
>> +{
>> + struct cec_notifier *n;
>> +
>> + if (WARN_ON(!adap))
>> + return NULL;
>> +
>> + n = cec_notifier_get_conn(hdmi_dev, conn_name);
>> + if (!n)
>> + return n;
>> +
>> + mutex_lock(&n->lock);
>> + n->cec_adap = adap;
>> + adap->conn_info = n->conn_info;
> Would it make sense to use cec_s_conn_info? There is a certain
> asymmetry here - cec_s_phys_addr is used to configure physical
> address, which also takes the adapter's lock while setting the
> physical address. That lock is not taken while the connector info is
> being set (not sure if that really matters for the code paths that
> would call into this function).
I thought about that, but there is a side-effect if I do that:
both cec_s_conn_info and cec_s_phys_addr send a state_changed
event, and I want to avoid that. Doing it this way ensures that
there will be only one event.
But there is definitely room for improvement here and I want to
work on that for the next kernel.
>
>> + adap->notifier = n;
>> + cec_s_phys_addr(adap, n->phys_addr, false);
>> + mutex_unlock(&n->lock);
>> + return n;
>> +}
>> +EXPORT_SYMBOL_GPL(cec_notifier_cec_adap_register);
>> +
>> +void cec_notifier_cec_adap_unregister(struct cec_notifier *n)
>> +{
>> + if (!n)
>> + return;
>> +
>> + mutex_lock(&n->lock);
>> + memset(&n->cec_adap->conn_info, 0, sizeof(n->cec_adap->conn_info));
> Could cec_s_conn_info be used to reset the connector info?
> Also, we explicitly clear connector info here. Since the notifier
> provides both connector info and physical address, maybe it would make
> sense to clear physical address as well?
Actually, this memset should be removed altogether. The connector info
doesn't change at all, it's just that right after this function is called
the whole CEC adapter will be unregistered. I thought that it might make
sense to zero the connector info, but really I should just leave it alone.
>
>
> ...
>> void cec_notifier_unregister(struct cec_notifier *n)
>> {
>> + /* Do nothing unless cec_notifier_register was called first */
>> + if (!n->called_cec_notifier_register)
> Could this check be made with n->lock held? cec_notifier_register sets
> this value while holding that lock.
It's not needed: you can never have a race here.
BTW, I'll be glad when I can remove these cec_notifier_(un)register
functions, they are pretty ugly and confusing.
Regards,
Hans
> ...
>
>
> Thank you.
>
