[v2,3/9] scsi: build scsi_common.o for all scsi passthrough request users

From: Christoph Hellwig <hch@lst.de>

Split scsi_common.o out of SCSI so that non-SCSI users can pull it in
easily for future sense buffer helper usage.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/Makefile      | 2 +-
 drivers/scsi/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

On Tue, 2018-07-31 at 12:51 -0700, Kees Cook wrote:
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 24cd47014657..a6abd7a856c6 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -76,7 +76,7 @@ obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
>  obj-$(CONFIG_NUBUS)		+= nubus/
>  obj-y				+= macintosh/
>  obj-$(CONFIG_IDE)		+= ide/
> -obj-$(CONFIG_SCSI)		+= scsi/
> +obj-y				+= scsi/
>  obj-y				+= nvme/
>  obj-$(CONFIG_ATA)		+= ata/
>  obj-$(CONFIG_TARGET_CORE)	+= target/

The above change not only selects scsi_common.o but also the following object
files: scsi.o hosts.o scsi_ioctl.o scsicam.o scsi_error.o scsi_lib.o scsi_scan.o
scsi_sysfs.o scsi_devinfo.o scsi_trace.o scsi_logging.o. That is a change that
has not been mentioned in the patch description. That makes me wonder whether
this is an intended change? Wouldn't a cleaner solution have been to move
scsi_common.c into a new directory?

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jul 31, 2018 at 08:01:16PM +0000, Bart Van Assche wrote:
> On Tue, 2018-07-31 at 12:51 -0700, Kees Cook wrote:
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 24cd47014657..a6abd7a856c6 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -76,7 +76,7 @@ obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
> >  obj-$(CONFIG_NUBUS)		+= nubus/
> >  obj-y				+= macintosh/
> >  obj-$(CONFIG_IDE)		+= ide/
> > -obj-$(CONFIG_SCSI)		+= scsi/
> > +obj-y				+= scsi/
> >  obj-y				+= nvme/
> >  obj-$(CONFIG_ATA)		+= ata/
> >  obj-$(CONFIG_TARGET_CORE)	+= target/
> 
> The above change not only selects scsi_common.o but also the following object
> files: scsi.o hosts.o scsi_ioctl.o scsicam.o scsi_error.o scsi_lib.o scsi_scan.o
> scsi_sysfs.o scsi_devinfo.o scsi_trace.o scsi_logging.o. That is a change that
> has not been mentioned in the patch description.

It shouldn't.  All these are built into scsi_mod.o, which is only built
when CONFIG_SCSI is set.  Under what circumstances do you see them built?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2018-07-31 at 13:12 -0700, hch@infradead.org wrote:
> It shouldn't.  All these are built into scsi_mod.o, which is only built
> when CONFIG_SCSI is set.  Under what circumstances do you see them built?

Although I think you are right, I still prefer that the scsi_common.c file
would be moved to a new directory. That will prevent confusion later on for
people who want to add additional code. This patch makes it nontrivial to
figure out which code is built when SCSI target functionality is enabled but
SCSI initiator functionality is not selected. I think moving scsi_common.c
into a new directory would make it much easier to figure out which code is
built depending on the kernel configuration.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jul 31, 2018 at 1:18 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Tue, 2018-07-31 at 13:12 -0700, hch@infradead.org wrote:
>> It shouldn't.  All these are built into scsi_mod.o, which is only built
>> when CONFIG_SCSI is set.  Under what circumstances do you see them built?
>
> Although I think you are right, I still prefer that the scsi_common.c file
> would be moved to a new directory. That will prevent confusion later on for
> people who want to add additional code. This patch makes it nontrivial to
> figure out which code is built when SCSI target functionality is enabled but
> SCSI initiator functionality is not selected. I think moving scsi_common.c
> into a new directory would make it much easier to figure out which code is
> built depending on the kernel configuration.

While I don't disagree with you, this series is the result of several
back-and-forth discussions where that option was seemingly rejected.
Christoph's approach seemed to satisfy Jens. If you can convince them,
sure! I'll do whatever the consensus is. :)

-Kees