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 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.