[PATCH Linux-2.6.18] scsifront: avoid aquiring same lock twice if ring is full

[PATCH Linux-2.6.18] scsifront: avoid aquiring same lock twice if ring is full

[Juergen Gross]

The locking in scsifront_dev_reset_handler() is obviously wrong. In
case of a full ring the host lock is aquired twice.

Fixing this issue enables to get rid of the endless fo loop with an
explicit break statement.

Signed-off-by: Juergen Gross <jgross@suse.com>
---

diff -r 078f1bb69ea5 drivers/xen/scsifront/scsifront.c
--- a/drivers/xen/scsifront/scsifront.c	Wed Dec 10 10:22:39 2014 +0100
+++ b/drivers/xen/scsifront/scsifront.c	Tue Jan 13 14:32:33 2015 +0100
@@ -447,12 +447,10 @@ static int scsifront_dev_reset_handler(s
 	uint16_t rqid;
 	int err = 0;
 
-	for (;;) {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
-		spin_lock_irq(host->host_lock);
+	spin_lock_irq(host->host_lock);
 #endif
-		if (!RING_FULL(&info->ring))
-			break;
+	while (RING_FULL(&info->ring)) {
 		if (err) {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
 			spin_unlock_irq(host->host_lock);

Re: [PATCH Linux-2.6.18] scsifront: avoid aquiring same lock twice if ring is full

[Pasi Kärkkäinen]

Hi,

On Tue, Jan 13, 2015 at 05:22:58PM +0100, Juergen Gross wrote:
> The locking in scsifront_dev_reset_handler() is obviously wrong. In
> case of a full ring the host lock is aquired twice.
> 
> Fixing this issue enables to get rid of the endless fo loop with an
> explicit break statement.
> 

Is this patch needed in upstream Linux kernel aswell, now that Xen PVSCSI drivers are in upstream Linux ?


Thanks,

-- Pasi


> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> 
> diff -r 078f1bb69ea5 drivers/xen/scsifront/scsifront.c
> --- a/drivers/xen/scsifront/scsifront.c	Wed Dec 10 10:22:39 2014 +0100
> +++ b/drivers/xen/scsifront/scsifront.c	Tue Jan 13 14:32:33 2015 +0100
> @@ -447,12 +447,10 @@ static int scsifront_dev_reset_handler(s
>  	uint16_t rqid;
>  	int err = 0;
>  
> -	for (;;) {
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
> -		spin_lock_irq(host->host_lock);
> +	spin_lock_irq(host->host_lock);
>  #endif
> -		if (!RING_FULL(&info->ring))
> -			break;
> +	while (RING_FULL(&info->ring)) {
>  		if (err) {
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
>  			spin_unlock_irq(host->host_lock);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH Linux-2.6.18] scsifront: avoid aquiring same lock twice if ring is full

[Juergen Gross]

On 01/13/2015 07:53 PM, Pasi Kärkkäinen wrote:
> Hi,
>
> On Tue, Jan 13, 2015 at 05:22:58PM +0100, Juergen Gross wrote:
>> The locking in scsifront_dev_reset_handler() is obviously wrong. In
>> case of a full ring the host lock is aquired twice.
>>
>> Fixing this issue enables to get rid of the endless fo loop with an
>> explicit break statement.
>>
>
> Is this patch needed in upstream Linux kernel aswell, now that Xen PVSCSI drivers are in upstream Linux ?

No, especially this part of the code was reorganized and doesn't
have that issue.

Juergen

>
>
> Thanks,
>
> -- Pasi
>
>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>
>> diff -r 078f1bb69ea5 drivers/xen/scsifront/scsifront.c
>> --- a/drivers/xen/scsifront/scsifront.c	Wed Dec 10 10:22:39 2014 +0100
>> +++ b/drivers/xen/scsifront/scsifront.c	Tue Jan 13 14:32:33 2015 +0100
>> @@ -447,12 +447,10 @@ static int scsifront_dev_reset_handler(s
>>   	uint16_t rqid;
>>   	int err = 0;
>>
>> -	for (;;) {
>>   #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
>> -		spin_lock_irq(host->host_lock);
>> +	spin_lock_irq(host->host_lock);
>>   #endif
>> -		if (!RING_FULL(&info->ring))
>> -			break;
>> +	while (RING_FULL(&info->ring)) {
>>   		if (err) {
>>   #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
>>   			spin_unlock_irq(host->host_lock);
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

Re: [PATCH Linux-2.6.18] scsifront: avoid aquiring same lock twice if ring is full

[Jan Beulich]

>>> On 13.01.15 at 17:22, <"jgross@suse.com".non-mime.internet> wrote:
> The locking in scsifront_dev_reset_handler() is obviously wrong. In
> case of a full ring the host lock is aquired twice.
> 
> Fixing this issue enables to get rid of the endless fo loop with an
> explicit break statement.

Perhaps worth noting that it was my c/s 1209:4d1471ca031e that
screwed this up.

Jan

> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> 
> diff -r 078f1bb69ea5 drivers/xen/scsifront/scsifront.c
> --- a/drivers/xen/scsifront/scsifront.c	Wed Dec 10 10:22:39 2014 +0100
> +++ b/drivers/xen/scsifront/scsifront.c	Tue Jan 13 14:32:33 2015 +0100
> @@ -447,12 +447,10 @@ static int scsifront_dev_reset_handler(s
>  	uint16_t rqid;
>  	int err = 0;
>  
> -	for (;;) {
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
> -		spin_lock_irq(host->host_lock);
> +	spin_lock_irq(host->host_lock);
>  #endif
> -		if (!RING_FULL(&info->ring))
> -			break;
> +	while (RING_FULL(&info->ring)) {
>  		if (err) {
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,12)
>  			spin_unlock_irq(host->host_lock);