[PATCH] sg: remove unreachable code in SET_FORCE_LOW_DMA

[PATCH] sg: remove unreachable code in SET_FORCE_LOW_DMA

[Hannes Reinecke]

The low_dma value is always '1' in that branch, so the remaining
'if' statement can never be reached.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/sg.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index dbe5b4b..652b934 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -890,14 +890,9 @@ static int max_sectors_bytes(struct request_queue *q)
 		result = get_user(val, ip);
 		if (result)
 			return result;
-		if (val) {
+		if (val)
 			sfp->low_dma = 1;
-			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
-				val = (int) sfp->reserve.bufflen;
-				sg_remove_scat(sfp, &sfp->reserve);
-				sg_build_reserve(sfp, val);
-			}
-		} else {
+		else {
 			if (atomic_read(&sdp->detaching))
 				return -ENODEV;
 			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
-- 
1.8.5.6

Re: [PATCH] sg: remove unreachable code in SET_FORCE_LOW_DMA

[Christoph Hellwig]

On Wed, Feb 01, 2017 at 12:26:39PM +0100, Hannes Reinecke wrote:
> The low_dma value is always '1' in that branch, so the remaining
> 'if' statement can never be reached.
>
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/sg.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index dbe5b4b..652b934 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -890,14 +890,9 @@ static int max_sectors_bytes(struct request_queue *q)
>  		result = get_user(val, ip);
>  		if (result)
>  			return result;
> -		if (val) {
> +		if (val)
>  			sfp->low_dma = 1;
> -			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
> -				val = (int) sfp->reserve.bufflen;
> -				sg_remove_scat(sfp, &sfp->reserve);
> -				sg_build_reserve(sfp, val);
> -			}
> -		} else {
> +		else {

I think the proper fix is to check sfp->low_dma for 0 before updating
it, at least that seems to be the intent here.

Re: [PATCH] sg: remove unreachable code in SET_FORCE_LOW_DMA

[Christoph Hellwig]

On Wed, Feb 01, 2017 at 02:00:48PM +0100, Christoph Hellwig wrote:
> I think the proper fix is to check sfp->low_dma for 0 before updating
> it, at least that seems to be the intent here.

Looking at this code a bit more I think the actual, real proper fix
is to remove the SET_FORCE_LOW_DMA entirely (except maybe the flag
for SG_GET_LOW_DMA and procfs output).  The block layer already does
bounce buffering if the DMA addressing constraints are not met,
so all this should just go away.

Btw, any rason Doug isn't on Cc on these sg.c patches?

Re: [PATCH] sg: remove unreachable code in SET_FORCE_LOW_DMA

[Hannes Reinecke]

On 02/01/2017 02:00 PM, Christoph Hellwig wrote:
> On Wed, Feb 01, 2017 at 12:26:39PM +0100, Hannes Reinecke wrote:
>> The low_dma value is always '1' in that branch, so the remaining
>> 'if' statement can never be reached.
>>
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/sg.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index dbe5b4b..652b934 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -890,14 +890,9 @@ static int max_sectors_bytes(struct request_queue *q)
>>  		result = get_user(val, ip);
>>  		if (result)
>>  			return result;
>> -		if (val) {
>> +		if (val)
>>  			sfp->low_dma = 1;
>> -			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
>> -				val = (int) sfp->reserve.bufflen;
>> -				sg_remove_scat(sfp, &sfp->reserve);
>> -				sg_build_reserve(sfp, val);
>> -			}
>> -		} else {
>> +		else {
> 
> I think the proper fix is to check sfp->low_dma for 0 before updating
> it, at least that seems to be the intent here.
> 
May. Or may not.

But this code path has _never_ been taken for the last ten years; this
particular code predates the initial git check-in from Linus.
Where would be the point of enabling it now?
(Especially given all the issues we're having with the sg driver ...)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] sg: remove unreachable code in SET_FORCE_LOW_DMA

[Hannes Reinecke]

On 02/01/2017 02:03 PM, Christoph Hellwig wrote:
> On Wed, Feb 01, 2017 at 02:00:48PM +0100, Christoph Hellwig wrote:
>> I think the proper fix is to check sfp->low_dma for 0 before updating
>> it, at least that seems to be the intent here.
> 
> Looking at this code a bit more I think the actual, real proper fix
> is to remove the SET_FORCE_LOW_DMA entirely (except maybe the flag
> for SG_GET_LOW_DMA and procfs output).  The block layer already does
> bounce buffering if the DMA addressing constraints are not met,
> so all this should just go away.
> 
Oh, I'm more than happy to do so.

> Btw, any rason Doug isn't on Cc on these sg.c patches?
> 
None. Oversight from my side.

Will be sending a v2 with those two patches rolled into one patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)