diff mbox series

[v4,03/27] ata: make SATA_PMP option selectable only if any SATA host driver is enabled

Message ID 20200317144333.2904-4-b.zolnierkie@samsung.com
State Not Applicable
Delegated to: David Miller
Headers show
Series [v4,01/27] ata: remove stale maintainership information from core code | expand

Commit Message

Bartlomiej Zolnierkiewicz March 17, 2020, 2:43 p.m. UTC 
There is no reason to expose SATA_PMP config option when no SATA
host drivers are enabled. To fix it add SATA_HOST config option,
make all SATA host drivers select it and finally make SATA_PMP
config options depend on it.

This also serves as preparation for the future changes which
optimize libata core code size on PATA only setups.

CC: "James E.J. Bottomley" <jejb@linux.ibm.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com> # for SCSI bits
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 drivers/ata/Kconfig         | 40 +++++++++++++++++++++++++++++++++++++
 drivers/scsi/Kconfig        |  1 +
 drivers/scsi/libsas/Kconfig |  1 +
 3 files changed, 42 insertions(+)

Comments

James Bottomley March 17, 2020, 2:55 p.m. UTC | #1 
On Tue, 2020-03-17 at 15:43 +0100, Bartlomiej Zolnierkiewicz wrote:
> There is no reason to expose SATA_PMP config option when no SATA
> host drivers are enabled. To fix it add SATA_HOST config option,
> make all SATA host drivers select it and finally make SATA_PMP
> config options depend on it.
> 
> This also serves as preparation for the future changes which
> optimize libata core code size on PATA only setups.
> 
> CC: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com> # for
> SCSI bits
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  drivers/ata/Kconfig         | 40
> +++++++++++++++++++++++++++++++++++++
>  drivers/scsi/Kconfig        |  1 +
>  drivers/scsi/libsas/Kconfig |  1 +
>  3 files changed, 42 insertions(+)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index a6beb2c5a692..ad7760656f71 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -34,6 +34,9 @@ if ATA
>  config ATA_NONSTANDARD
>         bool
>  
> +config SATA_HOST
> +	bool
> +
>  config ATA_VERBOSE_ERROR
>  	bool "Verbose ATA error reporting"
>  	default y
> @@ -73,6 +76,7 @@ config SATA_ZPODD
>  
>  config SATA_PMP
>  	bool "SATA Port Multiplier support"
> +	depends on SATA_HOST
>  	default y
>  	help
>  	  This option adds support for SATA Port Multipliers
> @@ -85,6 +89,7 @@ comment "Controllers with non-SFF native interface"
>  config SATA_AHCI
>  	tristate "AHCI SATA support"
>  	depends on PCI
> +	select SATA_HOST
>  	help
>  	  This option enables support for AHCI Serial ATA.

This is a bit fragile and not the way Kconfig should be done.  The
fragility comes because anyone adding a new host has also to remember
to add the select, and there will be no real consequences for not doing
so.  The way to get rid of the fragility is to make SATA_HOST a
menuconfig option enclosing all the hosts, which makes the patch much
smaller as well.  The hint implies you want to separate out all the
PATA drivers, which also makes a menuconfig sound like the better
option.

I've also got to say that the problem doesn't seem to be one ... even
if some raving lunatic disables all SATA hosts and then enables PMP it
doesn't cause any problems does it?

James
Bartlomiej Zolnierkiewicz March 19, 2020, 4:15 p.m. UTC | #2 
On 3/17/20 3:55 PM, James Bottomley wrote:
> On Tue, 2020-03-17 at 15:43 +0100, Bartlomiej Zolnierkiewicz wrote:
>> There is no reason to expose SATA_PMP config option when no SATA
>> host drivers are enabled. To fix it add SATA_HOST config option,
>> make all SATA host drivers select it and finally make SATA_PMP
>> config options depend on it.
>>
>> This also serves as preparation for the future changes which
>> optimize libata core code size on PATA only setups.
>>
>> CC: "James E.J. Bottomley" <jejb@linux.ibm.com>
>> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com> # for
>> SCSI bits
>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> ---
>>  drivers/ata/Kconfig         | 40
>> +++++++++++++++++++++++++++++++++++++
>>  drivers/scsi/Kconfig        |  1 +
>>  drivers/scsi/libsas/Kconfig |  1 +
>>  3 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index a6beb2c5a692..ad7760656f71 100644
>> --- a/drivers/ata/Kconfig
>> +++ b/drivers/ata/Kconfig
>> @@ -34,6 +34,9 @@ if ATA
>>  config ATA_NONSTANDARD
>>         bool
>>  
>> +config SATA_HOST
>> +	bool
>> +
>>  config ATA_VERBOSE_ERROR
>>  	bool "Verbose ATA error reporting"
>>  	default y
>> @@ -73,6 +76,7 @@ config SATA_ZPODD
>>  
>>  config SATA_PMP
>>  	bool "SATA Port Multiplier support"
>> +	depends on SATA_HOST
>>  	default y
>>  	help
>>  	  This option adds support for SATA Port Multipliers
>> @@ -85,6 +89,7 @@ comment "Controllers with non-SFF native interface"
>>  config SATA_AHCI
>>  	tristate "AHCI SATA support"
>>  	depends on PCI
>> +	select SATA_HOST
>>  	help
>>  	  This option enables support for AHCI Serial ATA.
> 
> This is a bit fragile and not the way Kconfig should be done.  The
> fragility comes because anyone adding a new host has also to remember
> to add the select, and there will be no real consequences for not doing

People adding the new host driver usually look at the existing Kconfig
entries before introducing the new one (most probably just copy and then
modify existing entry) so they should notice that the existing entries
contain the select.

Also we shouldn't have problem in catching potential select omissions
during the code review.

Not to mention that the addition of the new ATA host driver is quite
a rare event nowadays.. ;)

> so.  The way to get rid of the fragility is to make SATA_HOST a
> menuconfig option enclosing all the hosts, which makes the patch much
> smaller as well.  The hint implies you want to separate out all the
> PATA drivers, which also makes a menuconfig sound like the better
> option.
SATA_HOST is not for grouping SATA hosts, it is for core libata
code to enable SATA support (please see patches #13-26 for details,
maybe it should have been named ATA_SATA?) and menuconfig usage in
this case is problematic:

- we have host drivers supporting both SATA and PATA (i.e. ata_piix)

- we have currently host drivers sorted in Kconfig by different 
  criteria than SATA or PATA support

- I would prefer to avoid making users to have explicitly select
  SATA_HOST option (right now it gets auto-selected when needed)

- SCSI_SAS_ATA (which also needs to select SATA_HOST) lives in
  drivers/scsi/libsas/Kconfig so menuconfig will not cover it

> I've also got to say that the problem doesn't seem to be one ... even
> if some raving lunatic disables all SATA hosts and then enables PMP it
> doesn't cause any problems does it?

It doesn't cause any real problems (just some dead code being present
in the kernel image) but this patch is a prerequisite for patches #13-26
(patch description mentions that this change is needed for the future
changes which optimize libata core code size on PATA only setups).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Christoph Hellwig March 26, 2020, 9:46 a.m. UTC | #3 
On Tue, Mar 17, 2020 at 03:43:09PM +0100, Bartlomiej Zolnierkiewicz wrote:
> There is no reason to expose SATA_PMP config option when no SATA
> host drivers are enabled. To fix it add SATA_HOST config option,
> make all SATA host drivers select it and finally make SATA_PMP
> config options depend on it.
> 
> This also serves as preparation for the future changes which
> optimize libata core code size on PATA only setups.
> 
> CC: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com> # for SCSI bits
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index a6beb2c5a692..ad7760656f71 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -34,6 +34,9 @@  if ATA
 config ATA_NONSTANDARD
        bool
 
+config SATA_HOST
+	bool
+
 config ATA_VERBOSE_ERROR
 	bool "Verbose ATA error reporting"
 	default y
@@ -73,6 +76,7 @@  config SATA_ZPODD
 
 config SATA_PMP
 	bool "SATA Port Multiplier support"
+	depends on SATA_HOST
 	default y
 	help
 	  This option adds support for SATA Port Multipliers
@@ -85,6 +89,7 @@  comment "Controllers with non-SFF native interface"
 config SATA_AHCI
 	tristate "AHCI SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for AHCI Serial ATA.
 
@@ -111,6 +116,7 @@  config SATA_MOBILE_LPM_POLICY
 
 config SATA_AHCI_PLATFORM
 	tristate "Platform AHCI SATA support"
+	select SATA_HOST
 	help
 	  This option enables support for Platform AHCI Serial ATA
 	  controllers.
@@ -121,6 +127,7 @@  config AHCI_BRCM
 	tristate "Broadcom AHCI SATA support"
 	depends on ARCH_BRCMSTB || BMIPS_GENERIC || ARCH_BCM_NSP || \
 		   ARCH_BCM_63XX
+	select SATA_HOST
 	help
 	  This option enables support for the AHCI SATA3 controller found on
 	  Broadcom SoC's.
@@ -130,6 +137,7 @@  config AHCI_BRCM
 config AHCI_DA850
 	tristate "DaVinci DA850 AHCI SATA support"
 	depends on ARCH_DAVINCI_DA850
+	select SATA_HOST
 	help
 	  This option enables support for the DaVinci DA850 SoC's
 	  onboard AHCI SATA.
@@ -139,6 +147,7 @@  config AHCI_DA850
 config AHCI_DM816
 	tristate "DaVinci DM816 AHCI SATA support"
 	depends on ARCH_OMAP2PLUS
+	select SATA_HOST
 	help
 	  This option enables support for the DaVinci DM816 SoC's
 	  onboard AHCI SATA controller.
@@ -148,6 +157,7 @@  config AHCI_DM816
 config AHCI_ST
 	tristate "ST AHCI SATA support"
 	depends on ARCH_STI
+	select SATA_HOST
 	help
 	  This option enables support for ST AHCI SATA controller.
 
@@ -157,6 +167,7 @@  config AHCI_IMX
 	tristate "Freescale i.MX AHCI SATA support"
 	depends on MFD_SYSCON && (ARCH_MXC || COMPILE_TEST)
 	depends on (HWMON && (THERMAL || !THERMAL_OF)) || !HWMON
+	select SATA_HOST
 	help
 	  This option enables support for the Freescale i.MX SoC's
 	  onboard AHCI SATA.
@@ -166,6 +177,7 @@  config AHCI_IMX
 config AHCI_CEVA
 	tristate "CEVA AHCI SATA support"
 	depends on OF
+	select SATA_HOST
 	help
 	  This option enables support for the CEVA AHCI SATA.
 	  It can be found on the Xilinx Zynq UltraScale+ MPSoC.
@@ -176,6 +188,7 @@  config AHCI_MTK
 	tristate "MediaTek AHCI SATA support"
 	depends on ARCH_MEDIATEK
 	select MFD_SYSCON
+	select SATA_HOST
 	help
 	  This option enables support for the MediaTek SoC's
 	  onboard AHCI SATA controller.
@@ -185,6 +198,7 @@  config AHCI_MTK
 config AHCI_MVEBU
 	tristate "Marvell EBU AHCI SATA support"
 	depends on ARCH_MVEBU
+	select SATA_HOST
 	help
 	  This option enables support for the Marvebu EBU SoC's
 	  onboard AHCI SATA.
@@ -203,6 +217,7 @@  config AHCI_OCTEON
 config AHCI_SUNXI
 	tristate "Allwinner sunxi AHCI SATA support"
 	depends on ARCH_SUNXI
+	select SATA_HOST
 	help
 	  This option enables support for the Allwinner sunxi SoC's
 	  onboard AHCI SATA.
@@ -212,6 +227,7 @@  config AHCI_SUNXI
 config AHCI_TEGRA
 	tristate "NVIDIA Tegra AHCI SATA support"
 	depends on ARCH_TEGRA
+	select SATA_HOST
 	help
 	  This option enables support for the NVIDIA Tegra SoC's
 	  onboard AHCI SATA.
@@ -221,12 +237,14 @@  config AHCI_TEGRA
 config AHCI_XGENE
 	tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
 	depends on PHY_XGENE
+	select SATA_HOST
 	help
 	 This option enables support for APM X-Gene SoC SATA host controller.
 
 config AHCI_QORIQ
 	tristate "Freescale QorIQ AHCI SATA support"
 	depends on OF
+	select SATA_HOST
 	help
 	  This option enables support for the Freescale QorIQ AHCI SoC's
 	  onboard AHCI SATA.
@@ -236,6 +254,7 @@  config AHCI_QORIQ
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC
+	select SATA_HOST
 	help
 	  This option enables support for Freescale 3.0Gbps SATA controller.
 	  It can be found on MPC837x and MPC8315.
@@ -245,6 +264,7 @@  config SATA_FSL
 config SATA_GEMINI
 	tristate "Gemini SATA bridge support"
 	depends on ARCH_GEMINI || COMPILE_TEST
+	select SATA_HOST
 	default ARCH_GEMINI
 	help
 	  This enabled support for the FTIDE010 to SATA bridge
@@ -255,6 +275,7 @@  config SATA_GEMINI
 config SATA_AHCI_SEATTLE
 	tristate "AMD Seattle 6.0Gbps AHCI SATA host controller support"
 	depends on ARCH_SEATTLE
+	select SATA_HOST
 	help
 	 This option enables support for AMD Seattle SATA host controller.
 
@@ -263,12 +284,14 @@  config SATA_AHCI_SEATTLE
 config SATA_INIC162X
 	tristate "Initio 162x SATA support (Very Experimental)"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Initio 162x Serial ATA.
 
 config SATA_ACARD_AHCI
 	tristate "ACard AHCI variant (ATP 8620)"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Acard.
 
@@ -277,6 +300,7 @@  config SATA_ACARD_AHCI
 config SATA_SIL24
 	tristate "Silicon Image 3124/3132 SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Silicon Image 3124/3132 Serial ATA.
 
@@ -326,6 +350,7 @@  config PATA_OCTEON_CF
 config SATA_QSTOR
 	tristate "Pacific Digital SATA QStor support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Pacific Digital Serial ATA QStor.
 
@@ -334,6 +359,7 @@  config SATA_QSTOR
 config SATA_SX4
 	tristate "Promise SATA SX4 support (Experimental)"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Promise Serial ATA SX4.
 
@@ -357,6 +383,7 @@  comment "SATA SFF controllers with BMDMA"
 config ATA_PIIX
 	tristate "Intel ESB, ICH, PIIX3, PIIX4 PATA/SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for ICH5/6/7/8 Serial ATA
 	  and support for PATA on the Intel ESB/ICH/PIIX3/PIIX4 series
@@ -368,6 +395,7 @@  config SATA_DWC
 	tristate "DesignWare Cores SATA support"
 	depends on DMADEVICES
 	select GENERIC_PHY
+	select SATA_HOST
 	help
 	  This option enables support for the on-chip SATA controller of the
 	  AppliedMicro processor 460EX.
@@ -398,6 +426,7 @@  config SATA_DWC_VDEBUG
 config SATA_HIGHBANK
 	tristate "Calxeda Highbank SATA support"
 	depends on ARCH_HIGHBANK || COMPILE_TEST
+	select SATA_HOST
 	help
 	  This option enables support for the Calxeda Highbank SoC's
 	  onboard SATA.
@@ -409,6 +438,7 @@  config SATA_MV
 	depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
 		   ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
 	select GENERIC_PHY
+	select SATA_HOST
 	help
 	  This option enables support for the Marvell Serial ATA family.
 	  Currently supports 88SX[56]0[48][01] PCI(-X) chips,
@@ -419,6 +449,7 @@  config SATA_MV
 config SATA_NV
 	tristate "NVIDIA SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for NVIDIA Serial ATA.
 
@@ -427,6 +458,7 @@  config SATA_NV
 config SATA_PROMISE
 	tristate "Promise SATA TX2/TX4 support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Promise Serial ATA TX2/TX4.
 
@@ -435,6 +467,7 @@  config SATA_PROMISE
 config SATA_RCAR
 	tristate "Renesas R-Car SATA support"
 	depends on ARCH_RENESAS || COMPILE_TEST
+	select SATA_HOST
 	help
 	  This option enables support for Renesas R-Car Serial ATA.
 
@@ -443,6 +476,7 @@  config SATA_RCAR
 config SATA_SIL
 	tristate "Silicon Image SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Silicon Image Serial ATA.
 
@@ -452,6 +486,7 @@  config SATA_SIS
 	tristate "SiS 964/965/966/180 SATA support"
 	depends on PCI
 	select PATA_SIS
+	select SATA_HOST
 	help
 	  This option enables support for SiS Serial ATA on
 	  SiS 964/965/966/180 and Parallel ATA on SiS 180.
@@ -462,6 +497,7 @@  config SATA_SIS
 config SATA_SVW
 	tristate "ServerWorks Frodo / Apple K2 SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Broadcom/Serverworks/Apple K2
 	  SATA support.
@@ -471,6 +507,7 @@  config SATA_SVW
 config SATA_ULI
 	tristate "ULi Electronics SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for ULi Electronics SATA.
 
@@ -479,6 +516,7 @@  config SATA_ULI
 config SATA_VIA
 	tristate "VIA SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for VIA Serial ATA.
 
@@ -487,6 +525,7 @@  config SATA_VIA
 config SATA_VITESSE
 	tristate "VITESSE VSC-7174 / INTEL 31244 SATA support"
 	depends on PCI
+	select SATA_HOST
 	help
 	  This option enables support for Vitesse VSC7174 and Intel 31244 Serial ATA.
 
@@ -1113,6 +1152,7 @@  config PATA_ACPI
 config ATA_GENERIC
 	tristate "Generic ATA support"
 	depends on PCI && ATA_BMDMA
+	select SATA_HOST
 	help
 	  This option enables support for generic BIOS configured
 	  ATA controllers via the new ATA layer
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index b5be6f43ec3f..17feff174f57 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -980,6 +980,7 @@  config SCSI_SYM53C8XX_MMIO
 config SCSI_IPR
 	tristate "IBM Power Linux RAID adapter support"
 	depends on PCI && SCSI && ATA
+	select SATA_HOST
 	select FW_LOADER
 	select IRQ_POLL
 	select SGL_ALLOC
diff --git a/drivers/scsi/libsas/Kconfig b/drivers/scsi/libsas/Kconfig
index 5c6a5eff2f8e..052ee3a26f6e 100644
--- a/drivers/scsi/libsas/Kconfig
+++ b/drivers/scsi/libsas/Kconfig
@@ -19,6 +19,7 @@  config SCSI_SAS_ATA
 	bool "ATA support for libsas (requires libata)"
 	depends on SCSI_SAS_LIBSAS
 	depends on ATA = y || ATA = SCSI_SAS_LIBSAS
+	select SATA_HOST
 	help
 		Builds in ATA support into libsas.  Will necessitate
 		the loading of libata along with libsas.