The recent change to mark the input argument of sg_pcopy_from_buffer
had the unfortunate side-effect to cause a new warning in the
scsi_debug code:
drivers/scsi/scsi_debug.c: In function 'do_device_access':
drivers/scsi/scsi_debug.c:2376:8: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
func = sg_pcopy_from_buffer;
This patch attempts to avoid that warning without adding
evil type casts, but unfortunately makes the do_device_access
function a lot uglier in the process.
Signed-off-by: Arnd Bergmann <[email protected]>
Fixes: 5250326459 ("lib/scatterlist: mark input buffer parameters as 'const'")
---
I can't decide if this is actually a good idea, or if we should rather drop
the sg_pcopy_from_buffer() patch. Maybe someone else sees a better solution.
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1f8e2dc9c616..dd9f0d1574ef 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2363,17 +2363,13 @@ do_device_access(struct scsi_cmnd *scmd, u64 lba, u32 num, bool do_write)
u64 block, rest = 0;
struct scsi_data_buffer *sdb;
enum dma_data_direction dir;
- size_t (*func)(struct scatterlist *, unsigned int, void *, size_t,
- off_t);
if (do_write) {
sdb = scsi_out(scmd);
dir = DMA_TO_DEVICE;
- func = sg_pcopy_to_buffer;
} else {
sdb = scsi_in(scmd);
dir = DMA_FROM_DEVICE;
- func = sg_pcopy_from_buffer;
}
if (!sdb->length)
@@ -2385,17 +2381,29 @@ do_device_access(struct scsi_cmnd *scmd, u64 lba, u32 num, bool do_write)
if (block + num > sdebug_store_sectors)
rest = block + num - sdebug_store_sectors;
- ret = func(sdb->table.sgl, sdb->table.nents,
- fake_storep + (block * scsi_debug_sector_size),
- (num - rest) * scsi_debug_sector_size, 0);
+ if (do_write)
+ ret = sg_pcopy_to_buffer(sdb->table.sgl, sdb->table.nents,
+ fake_storep + (block * scsi_debug_sector_size),
+ (num - rest) * scsi_debug_sector_size, 0);
+ else
+ ret = sg_pcopy_from_buffer(sdb->table.sgl, sdb->table.nents,
+ fake_storep + (block * scsi_debug_sector_size),
+ (num - rest) * scsi_debug_sector_size, 0);
+
if (ret != (num - rest) * scsi_debug_sector_size)
return ret;
- if (rest) {
- ret += func(sdb->table.sgl, sdb->table.nents,
- fake_storep, rest * scsi_debug_sector_size,
- (num - rest) * scsi_debug_sector_size);
- }
+ if (!rest)
+ return ret;
+
+ if (do_write)
+ ret += sg_pcopy_to_buffer(sdb->table.sgl, sdb->table.nents,
+ fake_storep, rest * scsi_debug_sector_size,
+ (num - rest) * scsi_debug_sector_size);
+ else
+ ret += sg_pcopy_from_buffer(sdb->table.sgl, sdb->table.nents,
+ fake_storep, rest * scsi_debug_sector_size,
+ (num - rest) * scsi_debug_sector_size);
return ret;
}
On Tue, 19 May 2015 23:22:39 +0200 Arnd Bergmann <[email protected]> wrote:
> The recent change to mark the input argument of sg_pcopy_from_buffer
> had the unfortunate side-effect to cause a new warning in the
> scsi_debug code:
>
> drivers/scsi/scsi_debug.c: In function 'do_device_access':
> drivers/scsi/scsi_debug.c:2376:8: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
> func = sg_pcopy_from_buffer;
>
> This patch attempts to avoid that warning without adding
> evil type casts, but unfortunately makes the do_device_access
> function a lot uglier in the process.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: 5250326459 ("lib/scatterlist: mark input buffer parameters as 'const'")
> ---
>
> I can't decide if this is actually a good idea, or if we should rather drop
> the sg_pcopy_from_buffer() patch. Maybe someone else sees a better solution.
Could make do_device_access() call sg_copy_buffer() directly.
But yes, dropping the sg_pcopy_from/to_buffer changes is reasonable.
sg_copy_buffer() is bidirectional and that won't be changing, so
putting constified wrapeprs around it is kinda fake.
On Wednesday 20 May 2015 12:53:29 Andrew Morton wrote:
> On Tue, 19 May 2015 23:22:39 +0200 Arnd Bergmann <[email protected]> wrote:
> >
> > I can't decide if this is actually a good idea, or if we should rather drop
> > the sg_pcopy_from_buffer() patch. Maybe someone else sees a better solution.
>
> Could make do_device_access() call sg_copy_buffer() directly.
>
> But yes, dropping the sg_pcopy_from/to_buffer changes is reasonable.
> sg_copy_buffer() is bidirectional and that won't be changing, so
> putting constified wrapeprs around it is kinda fake.
Ok. The part I only saw now is that do_device_access() is the only user
of sg_pcopy_from_buffer(), so if that passes a non-const argument,
there is dropping the patch will be teh best solution.
Arnd
On 20/05/15 21:31, Arnd Bergmann wrote:
> On Wednesday 20 May 2015 12:53:29 Andrew Morton wrote:
>> On Tue, 19 May 2015 23:22:39 +0200 Arnd Bergmann <[email protected]> wrote:
>>>
>>> I can't decide if this is actually a good idea, or if we should rather drop
>>> the sg_pcopy_from_buffer() patch. Maybe someone else sees a better solution.
>>
>> Could make do_device_access() call sg_copy_buffer() directly.
>>
>> But yes, dropping the sg_pcopy_from/to_buffer changes is reasonable.
>> sg_copy_buffer() is bidirectional and that won't be changing, so
>> putting constified wrapeprs around it is kinda fake.
>
> Ok. The part I only saw now is that do_device_access() is the only user
> of sg_pcopy_from_buffer(), so if that passes a non-const argument,
> there is dropping the patch will be teh best solution.
>
> Arnd
do_device_access() may the only user of sg_pcopy_from_buffer() in the
-mm tree at the moment, but the const-patch was created because we were
using the sg_pcopy_{to,from}_buffer functions in new code in the i915
driver (published to the intel-gfx mailing list, but not yet folded into
the upstream versions). So quite soon it won't be the only user :)
The various sg_[p]copy_* wrappers all just supply trailing parameters
for the convenience of those who don't need (and don't want to deal
with) the full capabilities of the underlying sg_copy_buffer(). In
particular, we want the wrappers for the benefit of users that *don't*
use this flag-specifies-direction style (which I think is actually quite
rare and not really conducive to robust checking). The
separate-source-and-destination style seems much more common (cf. memcpy()).
And scsi_debug.c itself has functions fill_from_dev_buffer() and
fetch_to_dev_buffer() that call the separate sg_copy_{to,from}_buffer()
wrappers.
But since do_device_access() has the same parameter style as
sg_copy_buffer() (i.e. pointer parameters that may be either source or
destination, plus a flag to specify direction of transfer, as opposed to
one (const *) parameter for the input and a separate one for the
(non-const) destination), I think it quite reasonable that
do_device_access() should call sg_copy_buffer() directly rather than
going through one or other wrapper. In fact it simplifies the code
further; we can lose four lines and get rid of the function pointer
entirely, just by passing 'do_write' down to sg_copy_buffer(). See
attached patch :)
.Dave.
On Thursday 21 May 2015 12:09:58 Dave Gordon wrote:
> From b304c5a99ea260eac1cf98ced5f3c79c793ad4fd Mon Sep 17 00:00:00 2001
> From: Dave Gordon <[email protected]>
> Date: Thu, 21 May 2015 12:06:27 +0100
> Subject: [PATCH] scsi: resolve sg buffer const-ness issue
>
> do_device_access() takes a separate parameter to indicate the direction
> of data transfer, which it used to use to select the appropriate function
> out of sg_pcopy_{to,from}_buffer(). However these two functions now have
> different const-ness in their signatures, leading to compiler warnings.
>
> So this patch makes it bypass these wrappers and call the underlying
> function sg_copy_buffer() directly; this has the same calling style as
> do_device_access() i.e. a separate direction-of-transfer parameter and
> no pointers-to-const, so skipping the wrappers not only eliminates the
> warning, it also make the code simpler
>
> Signed-off-by: Dave Gordon <[email protected]>
> ---
Acked-by: Arnd Bergmann <[email protected]>