On Thu, Apr 04, 2024 at 06:50:26PM +0900, Damien Le Moal wrote:
> Commits 0077a504e1a4 ("ahci: asm1166: correct count of reported ports")
> and 9815e3961754 ("ahci: asm1064: correct count of reported ports")
> attempted to limit the ports of the ASM1166 and ASM1064 AHCI controllers
> to avoid long boot times caused by the fact that these adapters report
> a port map larger than the number of physical ports. The excess ports
> are "virtual" to hide port multiplier devices and probing these ports
> takes time. However, these commits caused a regression for user that do
s/user/users/
> use PMP devices, as the ATA devices connected to the PMP cannot be
> scanned. These commits have thus been reverted by commit 6cd8adc3e18
> ("ahci: asm1064: asm1166: don't limit reported ports") to allow the
> discovery of devices connected through a port multiplier. But this
> revert re-introduced the long boot times for users that do not use a
> port multiplier setup.
>
> This patch adds the mask_port_map ahci module parameter to allow users
> to manually specify port map masks for controllers. In the case of the
> ASMedia 1166 and 1064 controllers, users that do not have port
> multiplier devices can mask the excess virtual ports exposed by the
> controller to speedup port scanning, thus reducing boot time.
>
> The mask_port_map parameter accepts 2 different format:
s/format/formats/
> - mask_port_map=<mask>
> This applies the same mask to all AHCI controllers
> present in the system. This format is convenient for small systems
> that have only a single AHCI controller.
> - mask_port_map=<dev name>=<mask>,<dev name>=mask,...
Perhaps replace "dev name" with "pci_dev" or "pci_bdf".
> This applies the specified masks only to the device listed. The
> device name correspond to the string following "ahci" in probe
> kernel messages. E.g. for:
> ahci 0000:01:00.0: 2/2 ports implemented (port mask 0x3)
> the device name is "0000:01:00.0".
>
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> drivers/ata/ahci.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 88 insertions(+)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 562302e2e57c..3bc8626f3ba9 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -666,6 +666,90 @@ static int mobile_lpm_policy = -1;
> module_param(mobile_lpm_policy, int, 0644);
> MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
>
> +static char *ahci_mask_port_map;
> +module_param_named(mask_port_map, ahci_mask_port_map, charp, 0444);
> +MODULE_PARM_DESC(mask_port_map,
> + "Provide 32-bit port map masks to ignore controllers ports. "
> + "Valid values are: "
Looking at other MODULE_PARM_DESC, it appears that you can use \n in the string.
So perhaps "Valid values are:\n"
> + "<mask> to apply the same mask to all controller devices, "
> + "<dev0_name>=<mask0>,<dev1_name>=<mask1>,...' to specify a "
Perhaps add a \n after describing the first format.
> + "different mask per controller, where <dev_name> is the "
> + "controller host name as printed in the messages "
Perhaps replace "host name" with PCI device or PCI BDF.
Perhaps replace "dev_name" with pci_dev or pci_bdf.
> + "\"ahci xxxx:bus:dev.func: ...\"");
s/xxxx/domain/
> +
> +static void ahci_apply_port_map_mask(struct device *dev,
> + struct ahci_host_priv *hpriv, char *mask_s)
> +{
> + unsigned int mask;
> +
> + if (kstrtouint(mask_s, 0, &mask)) {
> + dev_err(dev, "Invalid port map mask\n");
> + return;
> + }
> +
> + if (mask) {
> + dev_warn(dev, "Forcing port map mask 0x%x\n", mask);
> + hpriv->mask_port_map = mask;
I think this should use saved_port_map instead of mask_port_map, see:
https://lore.kernel.org/linux-ide/uu2exwldqvbdjus6t4r3cxuto5jpeqtjfvc7qiikulfwiyntf3@j4btf2bt23ld/
""
1. saved_port_map defines the ports actually available on the host
controller.
2. mask_port_map masks out the unused ports if it's initialized,
otherwise all available ports will be initialized and utilized.
""
(We don't want to initialize them at all.)
Also, if you use saved_port_map, you don't need any print.
There will already be a print:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/ata/libahci.c#L537
Also, see:
566d1827df2e ("libata: disable forced PORTS_IMPL for >= AHCI 1.3")
A mask of 0 is valid, so I don't think that you can do
if (mask)
Perhaps just:
if (kstrtouint(mask_s, 0, &mask)) {
dev_err(dev, "Invalid port map mask\n");
return;
}
hpriv->saved_port_map = mask;
> + }
> +}
> +
> +static void ahci_get_port_map_mask(struct device *dev,
> + struct ahci_host_priv *hpriv)
> +{
> + char *param, *end, *str, *mask_s;
> + char *name;
> +
> + if (!strlen(ahci_mask_port_map))
> + return;
> +
> + str = kstrdup(ahci_mask_port_map, GFP_KERNEL);
> + if (!str)
> + return;
> +
> + if (!strchr(str, '=')) {
> + /* Single mask case */
> + ahci_apply_port_map_mask(dev, hpriv, str);
> + goto free;
> + }
> +
> + /*
> + * Mask list case: parse the parameter to apply the mask only if
> + * the device name matches.
> + */
> + param = str;
> + end = param + strlen(param);
> + while (param && param < end && *param) {
> + name = param;
> + param = strchr(name, '=');
> + if (!param)
> + break;
> +
> + *param = '\0';
> + param++;
> + if (param >= end)
> + break;
> +
> + if (strcmp(dev_name(dev), name) != 0) {
> + param = strchr(param, ',');
> + if (param)
> + param++;
> + continue;
> + }
> +
> + mask_s = param;
> + param = strchr(mask_s, ',');
> + if (param) {
> + *param = '\0';
> + param++;
> + }
> +
> + ahci_apply_port_map_mask(dev, hpriv, mask_s);
> + }
> +
> +free:
> + kfree(str);
> +}
> +
> static void ahci_pci_save_initial_config(struct pci_dev *pdev,
> struct ahci_host_priv *hpriv)
> {
> @@ -688,6 +772,10 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
> "Disabling your PATA port. Use the boot option 'ahci.marvell_enable=0' to avoid this.\n");
> }
>
> + /* Handle port map masks passed as module parameter. */
> + if (ahci_mask_port_map)
> + ahci_get_port_map_mask(&pdev->dev, hpriv);
> +
> ahci_save_initial_config(&pdev->dev, hpriv);
> }
>
> --
> 2.44.0
>
