Re: s390 common I/O layer locking

From: Vineeth Vijayan <vneethv@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>, Jason Gunthorpe <jgg@nvidia.com>
Cc: Eric Farman <farman@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	Peter Oberparleiter <oberpar@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, Christoph Hellwig <hch@lst.de>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: Re: s390 common I/O layer locking
Date: Tue, 4 May 2021 17:10:42 +0200	[thread overview]
Message-ID: <292442e8-3b1a-56c4-b974-05e8b358ba64@linux.ibm.com> (raw)
In-Reply-To: <20210503125440.0acd7c1f.cohuck@redhat.com>


On 5/3/21 12:54 PM, Cornelia Huck wrote:
> On Fri, 30 Apr 2021 14:19:08 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
>
>> On Fri, Apr 30, 2021 at 02:31:40PM +0200, Cornelia Huck wrote:
>>> On Thu, 29 Apr 2021 15:13:47 -0300
>>> Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>> All the checks for !private need some kind of locking. The driver core
>>>> model is that the 'struct device_driver' callbacks are all called
>>>> under the device_lock (this prevents the driver unbinding during the
>>>> callback). I didn't check if ccs does this or not..
>>> probe/remove/shutdown are basically a forward of the callbacks at the
>>> bus level.
>> These are all covered by device_lock
>>
>>> The css bus should make sure that we serialize
>>> irq/sch_event/chp_event with probe/remove.
>> Hum it doesn't look OK, like here:
>>
>> css_process_crw()
>>    css_evaluate_subchannel()
>>     sch = bus_find_device()
>>        -- So we have a refcount on the struct device
>>     css_evaluate_known_subchannel() {
>> 	if (sch->driver) {
>> 		if (sch->driver->sch_event)
>> 			ret = sch->driver->sch_event(sch, slow);
>>     }
>>
>> But the above call and touches to sch->driver (which is really just
>> sch->dev.driver) are unlocked and racy.
>>
>> I would hold the device_lock() over all touches to sch->driver outside
>> of a driver core callback.
> I think this issue did not come up much before, as most drivers on the
> css bus tend to stay put during the lifetime of the device; but yes, it
> seems we're missing some locking.
>
> For the css bus, we need locking for the event callbacks; for irq, this
> may interact with the subchannel lock and likely needs some care.
>
> I also looked at the other busses in the common I/O layer: scm looks
> good at a glance, ccwgroup and ccw have locking for online/offline; the
> other callbacks for the ccw drivers probably need to take the device
> lock as well.
>
> Common I/O layer maintainers, does that look right?
>
I just had a quick glance on the CIO layer drivers. And at first look, 
you are right.
It looks likewe need modifications in the event callbacks (referring css 
here)
Let me go thoughthis thoroughly and update.
Thank you.