| Message ID | 20240404095026.929491-1-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci: Add mask_port_map module parameter | expand |
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@kernel.org> > --- > 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 >
On 4/5/24 17:44, Niklas Cassel wrote: >> +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. Yes, I saw that in many places the description string is split using "\n". However, with that, the print of the parameter description with "modinfo ahci" is rather weird, with the parameter type (charp) ending up on its own line. I did not like it so I did not add any "\n". >> +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.) Correct, and they do not. The masked ports are using the dummy ops. So it works exactly as intended. This module argument defines a *mask* for a port map, not the port map to use. > 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 Hmm... Checking the code in ahci_save_initial_config(), we have: /* Override the HBA ports mapping if the platform needs it */ port_map = readl(mmio + HOST_PORTS_IMPL); if (hpriv->saved_port_map && port_map != hpriv->saved_port_map) { dev_info(dev, "forcing port_map 0x%lx -> 0x%x\n", port_map, hpriv->saved_port_map); port_map = hpriv->saved_port_map; } else { hpriv->saved_port_map = port_map; } if (hpriv->mask_port_map) { dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n", port_map, port_map & hpriv->mask_port_map); port_map &= hpriv->mask_port_map; } So by setting the mask_port_map, we *always* mask the port map, be it a forced one defined by saved_port_map, or the hardware reported one. The patch results in this: modrpobe ahci mask_port_map=0000:00:17.0=0x1 dmesg | grep ahci ... ahci 0000:00:17.0: Forcing port map mask 0x1 ahci 0000:00:17.0: masking port_map 0xff -> 0x1 ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode ahci 0000:00:17.0: (0000:00:17.0) 1/8 ports implemented (port mask 0x1) So I could remove the message I added I guess... I prefer that this module parameter defines a mask rather than a map as it is more general: it can be used for testing to override saved_port_map or masks already set for some controllers. > A mask of 0 is valid, so I don't think that you can do A mask of 0 would mean "no ports". So better off completely ignoring the controller for that case :) So no, I do not want that value to be valid. > > if (mask) > > Perhaps just: > > if (kstrtouint(mask_s, 0, &mask)) { > dev_err(dev, "Invalid port map mask\n"); > return; > } > > hpriv->saved_port_map = mask; See above. That is not necessarily better.
On Fri, Apr 05, 2024 at 09:24:09PM +0900, Damien Le Moal wrote: > On 4/5/24 17:44, Niklas Cassel wrote: > >> +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. > > Yes, I saw that in many places the description string is split using "\n". > However, with that, the print of the parameter description with "modinfo ahci" > is rather weird, with the parameter type (charp) ending up on its own line. I > did not like it so I did not add any "\n". Ok. > > >> +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.) > > Correct, and they do not. The masked ports are using the dummy ops. So it works > exactly as intended. This module argument defines a *mask* for a port map, not > the port map to use. Ok, as long as there is no real initialization, there should be no extra time spent during probing at boot, so using mask_port_map should be fine. > The patch results in this: > > modrpobe ahci mask_port_map=0000:00:17.0=0x1 > dmesg | grep ahci > ... > ahci 0000:00:17.0: Forcing port map mask 0x1 > ahci 0000:00:17.0: masking port_map 0xff -> 0x1 > ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode > ahci 0000:00:17.0: (0000:00:17.0) 1/8 ports implemented (port mask 0x1) > > So I could remove the message I added I guess... Right now, it does seem a bit redundant. I guess we could change the print to be more like: "overloading port map via kernel module param", but I presume that we don't have any prints for other module params, so I don't see why we should have one here, especially since we already print that the mask is forced. > I prefer that this module parameter defines a mask rather than a map as it is > more general: it can be used for testing to override saved_port_map or masks > already set for some controllers. > > > A mask of 0 is valid, so I don't think that you can do > > A mask of 0 would mean "no ports". So better off completely ignoring the > controller for that case :) So no, I do not want that value to be valid. Yes, and that is how it is defined in some controllers, see e.g.: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=566d1827df2e So I don't see why we shouldn't allow that. It would allow us to quickly "emulate" a controller with a port map that is all zeroes. (Which apparently do exist in the wild.) So it could be a quick way for us to test that the code correctly handles such controllers. Kind regards, Niklas
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: " + "<mask> to apply the same mask to all controller devices, " + "<dev0_name>=<mask0>,<dev1_name>=<mask1>,...' to specify a " + "different mask per controller, where <dev_name> is the " + "controller host name as printed in the messages " + "\"ahci xxxx:bus:dev.func: ...\""); + +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; + } +} + +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); }
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 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: - 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,... 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@kernel.org> --- drivers/ata/ahci.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)