Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Reply-To: pmorel@linux.ibm.com
Subject: Re: [PATCH v2 10/10] vfio: ccw: Let user wait when busy on IO
To:     Heiko Carstens <heiko.carstens@de.ibm.com>,
        Pierre Morel <pmorel@linux.vnet.ibm.com>
Cc:     pasic@linux.vnet.ibm.com, bjsdjshi@linux.vnet.ibm.com,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, cohuck@redhat.com
References: <1527243678-3140-1-git-send-email-pmorel@linux.vnet.ibm.com>
 <1527243678-3140-11-git-send-email-pmorel@linux.vnet.ibm.com>
 <20180525140418.GA17131@osiris>
From:   Pierre Morel <pmorel@linux.ibm.com>
Date:   Tue, 5 Jun 2018 15:02:06 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <20180525140418.GA17131@osiris>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Message-Id: <05ae3528-d4e9-3f0f-4585-8e209f9d9e22@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 25/05/2018 16:04, Heiko Carstens wrote:
> On Fri, May 25, 2018 at 12:21:18PM +0200, Pierre Morel wrote:
>> In the current implementation, we do not want to start a new SSCH
>> command before the last one ends.
>>
>> Currently the user needs to poll on the -EBUSY error to
>> wait before sending a new request.
>>
>> Let's be friendly with global warming and let the user sleep
>> until he may send a new request.
>>
>> Let's make the caller wait until the last SSCH ends.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_fsm.c     | 4 ++++
>>   drivers/s390/cio/vfio_ccw_ops.c     | 6 ++++++
>>   drivers/s390/cio/vfio_ccw_private.h | 1 +
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
>> index c37052d..97b74a1 100644
>> --- a/drivers/s390/cio/vfio_ccw_fsm.c
>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
>> @@ -200,6 +200,10 @@ static int fsm_irq(struct vfio_ccw_private *private)
>>   
>>   	if (private->io_trigger)
>>   		eventfd_signal(private->io_trigger, 1);
>> +
>> +	if (private->io_completion)
>> +		complete(private->io_completion);
>> +
>>   	return VFIO_CCW_STATE_IDLE;
>>   }
>>   
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index b202e73..39beb6e 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -183,6 +183,7 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>>   	struct vfio_ccw_private *private;
>>   	struct ccw_io_region *region;
>>   	union scsw *scsw;
>> +	DECLARE_COMPLETION_ONSTACK(completion);
>>   
>>   	if (*ppos + count > sizeof(*region))
>>   		return -EINVAL;
>> @@ -196,6 +197,11 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
>>   	scsw = (union scsw *) &region->scsw_area;
>>   	switch (scsw->cmd.fctl) {
>>   	case SCSW_FCTL_START_FUNC:
>> +		if (private->state == VFIO_CCW_STATE_BUSY) {
>> +			private->io_completion = &completion;
>> +			if (wait_for_completion_interruptible(&completion))
>> +				return -EINTR;
>> +		}
> What prevents a state change between checking the state and before
> private->io_completion is set? If that happens you would end with an
> endless wait.
>
> Similarly, you would have memory corruption if the task would be
> interrupted and if the function would be left, ending up with a stale
> private->io_completion completion pointer.
> The complete(private->io_completion) call will then write to a memory
> location that might already be reused.
>
> Just my 0.02 after having a very very short look ;)

Right, completely false, I should pay a little more (at least) attention.

Thanks to have had a very very short (but sharp)look.

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany