| Message ID | 20180731195155.46664-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Consolidate scsi sense buffer usage | expand |
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 target-devel" 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 target-devel" 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 target-devel" 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
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/ diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile index e29f9b8fd66d..1f6218b98430 100644 --- a/drivers/scsi/Makefile +++ b/drivers/scsi/Makefile @@ -21,6 +21,7 @@ CFLAGS_gdth.o = # -DDEBUG_GDTH=2 -D__SERIAL__ -D__COM2__ -DGDTH_STATISTICS obj-$(CONFIG_PCMCIA) += pcmcia/ obj-$(CONFIG_SCSI) += scsi_mod.o +obj-$(CONFIG_BLK_SCSI_REQUEST) += scsi_common.o obj-$(CONFIG_RAID_ATTRS) += raid_class.o @@ -155,7 +156,6 @@ obj-$(CONFIG_SCSI_HISI_SAS) += hisi_sas/ obj-$(CONFIG_SCSI_DEBUG) += scsi_debug.o scsi_mod-y += scsi.o hosts.o scsi_ioctl.o \ scsicam.o scsi_error.o scsi_lib.o -scsi_mod-y += scsi_common.o scsi_mod-$(CONFIG_SCSI_CONSTANTS) += constants.o scsi_mod-$(CONFIG_SCSI_DMA) += scsi_lib_dma.o scsi_mod-y += scsi_scan.o scsi_sysfs.o scsi_devinfo.o