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

[Bartlomiej Zolnierkiewicz]

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@xxxxxxxxxxxxx>
>> Reviewed-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx> # for
>> SCSI bits
>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>> ---
>>  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