linux-kernel - RE: [PATCH v7 4/7] i3c: master: add mastership handover support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM5PR07MB31963DACC77DCB2634010B27C1BE0@DM5PR07MB3196.namprd07.prod.outlook.com>
Date:   Tue, 12 May 2020 06:24:06 +0000
From:   Parshuram Raju Thombare <pthombar@...ence.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>,
        Przemyslaw Gaj <pgaj@...ence.com>
CC:     "bbrezillon@...nel.org" <bbrezillon@...nel.org>,
        "vitor.soares@...opsys.com" <vitor.soares@...opsys.com>,
        "linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Milind Parab <mparab@...ence.com>,
        "praneeth@...com" <praneeth@...com>
Subject: RE: [PATCH v7 4/7] i3c: master: add mastership handover support

>Those waits should be done in the master driver. Pass a timeout to
>->request_master() or make it a property of the i3c_master_controller
>if you like, but don't poll the status from the core.

Ok, I will move these pollings, check master has DA and MR done to 
master driver method request_mastership. Then we will not need
check_events method in master driver. Every master driver can
handle mastership request process in request_mastership method.

>> +	if (!ret)
>> +		m->bus.cur_master = m->this;
>> +
>> +mr_req_done:
>> +	i3c_bus_maintenance_unlock(&m->bus);
>> +	i3c_bus_normaluse_lock(&m->bus);
>
>You should downgrade the lock instead of releasing it. I really need to
>get my head around this locking scheme because I'm pretty sure we had
>good reasons for the locking/unlocking dance Przemek had in his series.

That locking/unlocking dance was apparently because caller of
acquire_bus holds read lock to avoid device on which it is 
operating does not disappear or get modified due to 
maintenance activities happening inside maintenance (write) lock.
Hence, here first we unlock normaluse lock and then gets
maintenance lock.

Przemek: Was there any other reason behind that ?

Yes, I agree it is safer to combine maintenance (write) unlock and then 
normaluse (read) lock, into downgrading maintenance lock to normaluse
lock in single step using downgrade_lock. I will make that change.

>> +
>> +	if (!master->secondary)
>> +		master->bus.cur_master = master->this;
>
>This change doesn't seem related to this patch. Looks like this should
>instead go to patch 3.

This is to initialize cur_master pointer in master object pointing to 
I3C object for current master on the bus. Only main master is by default 
current master at the beginning, so cur_master is initialized only
for the main master. And since set_info happens in bus_init during
master initialization this change is included in this patch.

>> +	/*
>> +	 * Maintenance lock and master check above is used to
>> +	 * avoid race amongst devices sending MR requests
>> +	 * at the same time, as soon as ENEC MST is sent by the current
>> +	 * master. It ensure that only one MR request is processed,
>> +	 * rest MR requests on losing devices will timeout in wait MR
>> +	 * DONE state. And next MR requests are blocked due to DISEC MST
>> +	 * sent by current master in yield operation.
>> +	 * New master should send ENEC MST once it's work is done.
>> +	 * maintainanace lock is also needed for i3c_master_get_accmst_locked.
>> +	 */
>> +
>> +	ret = i3c_master_disec_locked(m, I3C_BROADCAST_ADDR,
>> +				      I3C_CCC_EVENT_MR |
>> +				      I3C_CCC_EVENT_HJ);
>
>Isn't it taken care of at the controller level? I'm fine sending it
>here, but I suspect some controllers might actually auto-disable MRs
>once they receive one.

I think auto disable MR on receiving one, handles the case of multiple
devices sending MR requests at a same time. That is handled above using
maintenance lock and current master check.
DISEC MR, HJ is to avoid new master getting interrupted by MR or HJ
before it's use of bus is done. Once it's work is done ENEC MR, HJ is
sent using i3c_master_enable_mr_events(), may be we can
rename it to i3c_master_release_bus. It is not actual hand over of bus
but just allowing other master to acquire it. 
I am not disabling MR and HJ at the hardware level. But I can add that
in acquire_bus as one more level of safety, if DISEC is disregarded for
some reason by any device.

>	if (WARN_ON(m->this == m->bus.cur_master)) {
>
>And you should probably check that before send DISEC.

Sure, I will do that.

Regards,
Parshuram Thombare