| Message ID | 20241114155611.59577-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet | expand |
On 14/11/24 17:56, Hans de Goede wrote: > The Vexia Edu Atla 10 tablet distributed to schools in the Spanish > Andalucía region has no ACPI fwnode associated with the SDHCI controller > for its microsd-slot and thus has no ACPI GPIO resource info. > > This causes the following error to be logged and the slot to not work: > [ 10.572113] sdhci-pci 0000:00:12.0: failed to setup card detect gpio > > Add a DMI quirk table for providing gpiod_lookup_tables with manually > provided CD GPIO info and use this DMI table to provide the CD GPIO info > on this tablet. This fixes the microsd-slot not working. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v3: > - Add a cd_gpio_override pointer to sdhci_pci_fixes > - Add sdhci_pci_add_gpio_lookup_table() helper which kmemdup-s a const > struct gpiod_lookup_table to avoid races when using async probing > > Changes in v2: > - Make sdhci_pci_dmi_cd_gpio_overrides static const instead of just const > - Drop duplicate #include <linux/dmi.h> (already there at the end) > --- > drivers/mmc/host/sdhci-pci-core.c | 67 +++++++++++++++++++++++++++++++ > drivers/mmc/host/sdhci-pci.h | 1 + > 2 files changed, 68 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c > index ed45ed0bdafd..a2ddbe3d8742 100644 > --- a/drivers/mmc/host/sdhci-pci-core.c > +++ b/drivers/mmc/host/sdhci-pci-core.c > @@ -21,6 +21,7 @@ > #include <linux/io.h> > #include <linux/iopoll.h> > #include <linux/gpio.h> > +#include <linux/gpio/machine.h> > #include <linux/pm_runtime.h> > #include <linux/pm_qos.h> > #include <linux/debugfs.h> > @@ -1235,6 +1236,29 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = { > .priv_size = sizeof(struct intel_host), > }; > > +/* DMI quirks for devices with missing or broken CD GPIO info */ > +static const struct gpiod_lookup_table vexia_edu_atla10_cd_gpios = { > + .dev_id = "0000:00:12.0", > + .table = { > + GPIO_LOOKUP("INT33FC:00", 38, "cd", GPIO_ACTIVE_HIGH), > + { } > + }, > +}; This is good but I feel like we should make it more difficult to get the size wrong. Could introduce another struct to hold the size: struct sdhci_pci_gpio_data { const struct gpiod_lookup_table *gpios; size_t size; }; static const struct sdhci_pci_gpio_data vexia_edu_atla10_cd_gpio_data = { .gpios = &vexia_edu_atla10_cd_gpios, .size = sizeof(vexia_edu_atla10_cd_gpios), }; So: .driver_data = (void *)&vexia_edu_atla10_cd_gpio_data, and struct sdhci_pci_gpio_data *data; ... data = dmi_id->driver_data; cd_gpio_lookup_table = kmemdup(data->gpios, data->size, GFP_KERNEL); > + > +static const struct dmi_system_id sdhci_intel_byt_cd_gpio_override[] = { > + { > + /* Vexia Edu Atla 10 tablet 9V version */ > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), > + DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"), > + /* Above strings are too generic, also match on BIOS date */ > + DMI_MATCH(DMI_BIOS_DATE, "08/25/2014"), > + }, > + .driver_data = (void *)&vexia_edu_atla10_cd_gpios, > + }, > + { } > +}; > + > static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { > #ifdef CONFIG_PM_SLEEP > .resume = byt_resume, > @@ -1253,6 +1277,7 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { > .add_host = byt_add_host, > .remove_slot = byt_remove_slot, > .ops = &sdhci_intel_byt_ops, > + .cd_gpio_override = sdhci_intel_byt_cd_gpio_override, > .priv_size = sizeof(struct intel_host), > }; > > @@ -2054,6 +2079,37 @@ static const struct dev_pm_ops sdhci_pci_pm_ops = { > * * > \*****************************************************************************/ > > +static struct gpiod_lookup_table *sdhci_pci_add_gpio_lookup_table( > + struct sdhci_pci_chip *chip) Let's not line wrap until 100 columns > +{ > + struct gpiod_lookup_table *cd_gpio_lookup_table; > + const struct dmi_system_id *dmi_id = NULL; > + > + if (chip->fixes && chip->fixes->cd_gpio_override) > + dmi_id = dmi_first_match(chip->fixes->cd_gpio_override); > + > + if (!dmi_id) > + return NULL; > + > + cd_gpio_lookup_table = kmemdup(dmi_id->driver_data, > + /* 1 GPIO lookup entry + 1 terminating entry */ > + struct_size(cd_gpio_lookup_table, table, 2), > + GFP_KERNEL); > + if (!cd_gpio_lookup_table) > + return ERR_PTR(-ENOMEM); > + > + gpiod_add_lookup_table(cd_gpio_lookup_table); > + return cd_gpio_lookup_table; > +} > + > +static void sdhci_pci_remove_gpio_lookup_table(struct gpiod_lookup_table *lookup_table) > +{ > + if (lookup_table) { > + gpiod_remove_lookup_table(lookup_table); > + kfree(lookup_table); > + } > +} > + > static struct sdhci_pci_slot *sdhci_pci_probe_slot( > struct pci_dev *pdev, struct sdhci_pci_chip *chip, int first_bar, > int slotno) > @@ -2129,8 +2185,19 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( > device_init_wakeup(&pdev->dev, true); > > if (slot->cd_idx >= 0) { > + struct gpiod_lookup_table *cd_gpio_lookup_table; > + > + cd_gpio_lookup_table = sdhci_pci_add_gpio_lookup_table(chip); > + if (IS_ERR(cd_gpio_lookup_table)) { > + ret = PTR_ERR(cd_gpio_lookup_table); > + goto remove; > + } > + > ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, > slot->cd_override_level, 0); > + > + sdhci_pci_remove_gpio_lookup_table(cd_gpio_lookup_table); > + > if (ret && ret != -EPROBE_DEFER) > ret = mmc_gpiod_request_cd(host->mmc, NULL, > slot->cd_idx, > diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h > index 153704f812ed..4973fa859217 100644 > --- a/drivers/mmc/host/sdhci-pci.h > +++ b/drivers/mmc/host/sdhci-pci.h > @@ -156,6 +156,7 @@ struct sdhci_pci_fixes { > #endif > > const struct sdhci_ops *ops; > + const struct dmi_system_id *cd_gpio_override; > size_t priv_size; > }; >
Hi Adrian, On 15-Nov-24 8:33 AM, Adrian Hunter wrote: > On 14/11/24 17:56, Hans de Goede wrote: >> The Vexia Edu Atla 10 tablet distributed to schools in the Spanish >> Andalucía region has no ACPI fwnode associated with the SDHCI controller >> for its microsd-slot and thus has no ACPI GPIO resource info. >> >> This causes the following error to be logged and the slot to not work: >> [ 10.572113] sdhci-pci 0000:00:12.0: failed to setup card detect gpio >> >> Add a DMI quirk table for providing gpiod_lookup_tables with manually >> provided CD GPIO info and use this DMI table to provide the CD GPIO info >> on this tablet. This fixes the microsd-slot not working. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v3: >> - Add a cd_gpio_override pointer to sdhci_pci_fixes >> - Add sdhci_pci_add_gpio_lookup_table() helper which kmemdup-s a const >> struct gpiod_lookup_table to avoid races when using async probing >> >> Changes in v2: >> - Make sdhci_pci_dmi_cd_gpio_overrides static const instead of just const >> - Drop duplicate #include <linux/dmi.h> (already there at the end) >> --- >> drivers/mmc/host/sdhci-pci-core.c | 67 +++++++++++++++++++++++++++++++ >> drivers/mmc/host/sdhci-pci.h | 1 + >> 2 files changed, 68 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c >> index ed45ed0bdafd..a2ddbe3d8742 100644 >> --- a/drivers/mmc/host/sdhci-pci-core.c >> +++ b/drivers/mmc/host/sdhci-pci-core.c >> @@ -21,6 +21,7 @@ >> #include <linux/io.h> >> #include <linux/iopoll.h> >> #include <linux/gpio.h> >> +#include <linux/gpio/machine.h> >> #include <linux/pm_runtime.h> >> #include <linux/pm_qos.h> >> #include <linux/debugfs.h> >> @@ -1235,6 +1236,29 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = { >> .priv_size = sizeof(struct intel_host), >> }; >> >> +/* DMI quirks for devices with missing or broken CD GPIO info */ >> +static const struct gpiod_lookup_table vexia_edu_atla10_cd_gpios = { >> + .dev_id = "0000:00:12.0", >> + .table = { >> + GPIO_LOOKUP("INT33FC:00", 38, "cd", GPIO_ACTIVE_HIGH), >> + { } >> + }, >> +}; > > This is good but I feel like we should make it more difficult > to get the size wrong. Could introduce another struct to hold > the size: > > struct sdhci_pci_gpio_data { > const struct gpiod_lookup_table *gpios; > size_t size; > }; > > static const struct sdhci_pci_gpio_data vexia_edu_atla10_cd_gpio_data = { > .gpios = &vexia_edu_atla10_cd_gpios, > .size = sizeof(vexia_edu_atla10_cd_gpios), > }; > > So: > .driver_data = (void *)&vexia_edu_atla10_cd_gpio_data, > and > struct sdhci_pci_gpio_data *data; > ... > data = dmi_id->driver_data; > > cd_gpio_lookup_table = kmemdup(data->gpios, data->size, GFP_KERNEL); Interesting idea. But I'm afraid that sizeof(variable-name) on a struct with a flexible array member returns the same as just sizeof(struct struct-name) I added the following debug print to verify this: static int byt_sd_probe_slot(struct sdhci_pci_slot *slot) { + pr_info("sizeof(vexia_edu_atla10_cd_gpios) %lu sizeof(struct gpiod_lookup_table) %lu\n", + sizeof(vexia_edu_atla10_cd_gpios), sizeof(struct gpiod_lookup_table)); byt_probe_slot(slot); slot->host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_AGGRESSIVE_PM | MMC_CAP_CD_WAKE; And that prints: [ 10.459681] sizeof(vexia_edu_atla10_cd_gpios) 24 sizeof(struct gpiod_lookup_table) 24 So using sizeof(vexia_edu_atla10_cd_gpios) to get the size including the 2 flexible array members does not work since sizeof() does not take into account the size of the flexible array members. Regards, Hans > >> + >> +static const struct dmi_system_id sdhci_intel_byt_cd_gpio_override[] = { >> + { >> + /* Vexia Edu Atla 10 tablet 9V version */ >> + .matches = { >> + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), >> + DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"), >> + /* Above strings are too generic, also match on BIOS date */ >> + DMI_MATCH(DMI_BIOS_DATE, "08/25/2014"), >> + }, >> + .driver_data = (void *)&vexia_edu_atla10_cd_gpios, >> + }, >> + { } >> +}; >> + >> static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >> #ifdef CONFIG_PM_SLEEP >> .resume = byt_resume, >> @@ -1253,6 +1277,7 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >> .add_host = byt_add_host, >> .remove_slot = byt_remove_slot, >> .ops = &sdhci_intel_byt_ops, >> + .cd_gpio_override = sdhci_intel_byt_cd_gpio_override, >> .priv_size = sizeof(struct intel_host), >> }; >> >> @@ -2054,6 +2079,37 @@ static const struct dev_pm_ops sdhci_pci_pm_ops = { >> * * >> \*****************************************************************************/ >> >> +static struct gpiod_lookup_table *sdhci_pci_add_gpio_lookup_table( >> + struct sdhci_pci_chip *chip) > > Let's not line wrap until 100 columns > >> +{ >> + struct gpiod_lookup_table *cd_gpio_lookup_table; >> + const struct dmi_system_id *dmi_id = NULL; >> + >> + if (chip->fixes && chip->fixes->cd_gpio_override) >> + dmi_id = dmi_first_match(chip->fixes->cd_gpio_override); >> + >> + if (!dmi_id) >> + return NULL; >> + >> + cd_gpio_lookup_table = kmemdup(dmi_id->driver_data, >> + /* 1 GPIO lookup entry + 1 terminating entry */ >> + struct_size(cd_gpio_lookup_table, table, 2), >> + GFP_KERNEL); >> + if (!cd_gpio_lookup_table) >> + return ERR_PTR(-ENOMEM); >> + >> + gpiod_add_lookup_table(cd_gpio_lookup_table); >> + return cd_gpio_lookup_table; >> +} >> + >> +static void sdhci_pci_remove_gpio_lookup_table(struct gpiod_lookup_table *lookup_table) >> +{ >> + if (lookup_table) { >> + gpiod_remove_lookup_table(lookup_table); >> + kfree(lookup_table); >> + } >> +} >> + >> static struct sdhci_pci_slot *sdhci_pci_probe_slot( >> struct pci_dev *pdev, struct sdhci_pci_chip *chip, int first_bar, >> int slotno) >> @@ -2129,8 +2185,19 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( >> device_init_wakeup(&pdev->dev, true); >> >> if (slot->cd_idx >= 0) { >> + struct gpiod_lookup_table *cd_gpio_lookup_table; >> + >> + cd_gpio_lookup_table = sdhci_pci_add_gpio_lookup_table(chip); >> + if (IS_ERR(cd_gpio_lookup_table)) { >> + ret = PTR_ERR(cd_gpio_lookup_table); >> + goto remove; >> + } >> + >> ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, >> slot->cd_override_level, 0); >> + >> + sdhci_pci_remove_gpio_lookup_table(cd_gpio_lookup_table); >> + >> if (ret && ret != -EPROBE_DEFER) >> ret = mmc_gpiod_request_cd(host->mmc, NULL, >> slot->cd_idx, >> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h >> index 153704f812ed..4973fa859217 100644 >> --- a/drivers/mmc/host/sdhci-pci.h >> +++ b/drivers/mmc/host/sdhci-pci.h >> @@ -156,6 +156,7 @@ struct sdhci_pci_fixes { >> #endif >> >> const struct sdhci_ops *ops; >> + const struct dmi_system_id *cd_gpio_override; >> size_t priv_size; >> }; >> >
On 16/11/24 13:25, Hans de Goede wrote: > Hi Adrian, > > On 15-Nov-24 8:33 AM, Adrian Hunter wrote: >> On 14/11/24 17:56, Hans de Goede wrote: >>> The Vexia Edu Atla 10 tablet distributed to schools in the Spanish >>> Andalucía region has no ACPI fwnode associated with the SDHCI controller >>> for its microsd-slot and thus has no ACPI GPIO resource info. >>> >>> This causes the following error to be logged and the slot to not work: >>> [ 10.572113] sdhci-pci 0000:00:12.0: failed to setup card detect gpio >>> >>> Add a DMI quirk table for providing gpiod_lookup_tables with manually >>> provided CD GPIO info and use this DMI table to provide the CD GPIO info >>> on this tablet. This fixes the microsd-slot not working. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> Changes in v3: >>> - Add a cd_gpio_override pointer to sdhci_pci_fixes >>> - Add sdhci_pci_add_gpio_lookup_table() helper which kmemdup-s a const >>> struct gpiod_lookup_table to avoid races when using async probing >>> >>> Changes in v2: >>> - Make sdhci_pci_dmi_cd_gpio_overrides static const instead of just const >>> - Drop duplicate #include <linux/dmi.h> (already there at the end) >>> --- >>> drivers/mmc/host/sdhci-pci-core.c | 67 +++++++++++++++++++++++++++++++ >>> drivers/mmc/host/sdhci-pci.h | 1 + >>> 2 files changed, 68 insertions(+) >>> >>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c >>> index ed45ed0bdafd..a2ddbe3d8742 100644 >>> --- a/drivers/mmc/host/sdhci-pci-core.c >>> +++ b/drivers/mmc/host/sdhci-pci-core.c >>> @@ -21,6 +21,7 @@ >>> #include <linux/io.h> >>> #include <linux/iopoll.h> >>> #include <linux/gpio.h> >>> +#include <linux/gpio/machine.h> >>> #include <linux/pm_runtime.h> >>> #include <linux/pm_qos.h> >>> #include <linux/debugfs.h> >>> @@ -1235,6 +1236,29 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = { >>> .priv_size = sizeof(struct intel_host), >>> }; >>> >>> +/* DMI quirks for devices with missing or broken CD GPIO info */ >>> +static const struct gpiod_lookup_table vexia_edu_atla10_cd_gpios = { >>> + .dev_id = "0000:00:12.0", >>> + .table = { >>> + GPIO_LOOKUP("INT33FC:00", 38, "cd", GPIO_ACTIVE_HIGH), >>> + { } >>> + }, >>> +}; >> >> This is good but I feel like we should make it more difficult >> to get the size wrong. Could introduce another struct to hold >> the size: >> >> struct sdhci_pci_gpio_data { >> const struct gpiod_lookup_table *gpios; >> size_t size; >> }; >> >> static const struct sdhci_pci_gpio_data vexia_edu_atla10_cd_gpio_data = { >> .gpios = &vexia_edu_atla10_cd_gpios, >> .size = sizeof(vexia_edu_atla10_cd_gpios), >> }; >> >> So: >> .driver_data = (void *)&vexia_edu_atla10_cd_gpio_data, >> and >> struct sdhci_pci_gpio_data *data; >> ... >> data = dmi_id->driver_data; >> >> cd_gpio_lookup_table = kmemdup(data->gpios, data->size, GFP_KERNEL); > > Interesting idea. But I'm afraid that sizeof(variable-name) on a struct > with a flexible array member returns the same as just sizeof(struct struct-name) > I added the following debug print to verify this: > > static int byt_sd_probe_slot(struct sdhci_pci_slot *slot) > { > + pr_info("sizeof(vexia_edu_atla10_cd_gpios) %lu sizeof(struct gpiod_lookup_table) %lu\n", > + sizeof(vexia_edu_atla10_cd_gpios), sizeof(struct gpiod_lookup_table)); > byt_probe_slot(slot); > slot->host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | > MMC_CAP_AGGRESSIVE_PM | MMC_CAP_CD_WAKE; > > And that prints: > > [ 10.459681] sizeof(vexia_edu_atla10_cd_gpios) 24 sizeof(struct gpiod_lookup_table) 24 > > So using sizeof(vexia_edu_atla10_cd_gpios) to get the size including the 2 > flexible array members does not work since sizeof() does not take into > account the size of the flexible array members. Thanks for spotting that! Perhaps we should check the table size then? e.g. struct gpiod_lookup_table *table; size_t count; ... table = dmi_id->driver_data; for (count = 0; table->table[count].key; count++) ; if (count != 1) return ERR_PTR(-EINVAL); >> >>> + >>> +static const struct dmi_system_id sdhci_intel_byt_cd_gpio_override[] = { >>> + { >>> + /* Vexia Edu Atla 10 tablet 9V version */ >>> + .matches = { >>> + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), >>> + DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"), >>> + /* Above strings are too generic, also match on BIOS date */ >>> + DMI_MATCH(DMI_BIOS_DATE, "08/25/2014"), >>> + }, >>> + .driver_data = (void *)&vexia_edu_atla10_cd_gpios, >>> + }, >>> + { } >>> +}; >>> + >>> static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>> #ifdef CONFIG_PM_SLEEP >>> .resume = byt_resume, >>> @@ -1253,6 +1277,7 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>> .add_host = byt_add_host, >>> .remove_slot = byt_remove_slot, >>> .ops = &sdhci_intel_byt_ops, >>> + .cd_gpio_override = sdhci_intel_byt_cd_gpio_override, >>> .priv_size = sizeof(struct intel_host), >>> }; >>> >>> @@ -2054,6 +2079,37 @@ static const struct dev_pm_ops sdhci_pci_pm_ops = { >>> * * >>> \*****************************************************************************/ >>> >>> +static struct gpiod_lookup_table *sdhci_pci_add_gpio_lookup_table( >>> + struct sdhci_pci_chip *chip) >> >> Let's not line wrap until 100 columns >> >>> +{ >>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>> + const struct dmi_system_id *dmi_id = NULL; >>> + >>> + if (chip->fixes && chip->fixes->cd_gpio_override) >>> + dmi_id = dmi_first_match(chip->fixes->cd_gpio_override); >>> + >>> + if (!dmi_id) >>> + return NULL; >>> + >>> + cd_gpio_lookup_table = kmemdup(dmi_id->driver_data, >>> + /* 1 GPIO lookup entry + 1 terminating entry */ >>> + struct_size(cd_gpio_lookup_table, table, 2), >>> + GFP_KERNEL); >>> + if (!cd_gpio_lookup_table) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + gpiod_add_lookup_table(cd_gpio_lookup_table); >>> + return cd_gpio_lookup_table; >>> +} >>> + >>> +static void sdhci_pci_remove_gpio_lookup_table(struct gpiod_lookup_table *lookup_table) >>> +{ >>> + if (lookup_table) { >>> + gpiod_remove_lookup_table(lookup_table); >>> + kfree(lookup_table); >>> + } >>> +} >>> + >>> static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>> struct pci_dev *pdev, struct sdhci_pci_chip *chip, int first_bar, >>> int slotno) >>> @@ -2129,8 +2185,19 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>> device_init_wakeup(&pdev->dev, true); >>> >>> if (slot->cd_idx >= 0) { >>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>> + >>> + cd_gpio_lookup_table = sdhci_pci_add_gpio_lookup_table(chip); >>> + if (IS_ERR(cd_gpio_lookup_table)) { >>> + ret = PTR_ERR(cd_gpio_lookup_table); >>> + goto remove; >>> + } >>> + >>> ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, >>> slot->cd_override_level, 0); >>> + >>> + sdhci_pci_remove_gpio_lookup_table(cd_gpio_lookup_table); >>> + >>> if (ret && ret != -EPROBE_DEFER) >>> ret = mmc_gpiod_request_cd(host->mmc, NULL, >>> slot->cd_idx, >>> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h >>> index 153704f812ed..4973fa859217 100644 >>> --- a/drivers/mmc/host/sdhci-pci.h >>> +++ b/drivers/mmc/host/sdhci-pci.h >>> @@ -156,6 +156,7 @@ struct sdhci_pci_fixes { >>> #endif >>> >>> const struct sdhci_ops *ops; >>> + const struct dmi_system_id *cd_gpio_override; >>> size_t priv_size; >>> }; >>> >> >
Hi, On 18-Nov-24 11:10 AM, Adrian Hunter wrote: > On 16/11/24 13:25, Hans de Goede wrote: >> Hi Adrian, >> >> On 15-Nov-24 8:33 AM, Adrian Hunter wrote: >>> On 14/11/24 17:56, Hans de Goede wrote: >>>> The Vexia Edu Atla 10 tablet distributed to schools in the Spanish >>>> Andalucía region has no ACPI fwnode associated with the SDHCI controller >>>> for its microsd-slot and thus has no ACPI GPIO resource info. >>>> >>>> This causes the following error to be logged and the slot to not work: >>>> [ 10.572113] sdhci-pci 0000:00:12.0: failed to setup card detect gpio >>>> >>>> Add a DMI quirk table for providing gpiod_lookup_tables with manually >>>> provided CD GPIO info and use this DMI table to provide the CD GPIO info >>>> on this tablet. This fixes the microsd-slot not working. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> Changes in v3: >>>> - Add a cd_gpio_override pointer to sdhci_pci_fixes >>>> - Add sdhci_pci_add_gpio_lookup_table() helper which kmemdup-s a const >>>> struct gpiod_lookup_table to avoid races when using async probing >>>> >>>> Changes in v2: >>>> - Make sdhci_pci_dmi_cd_gpio_overrides static const instead of just const >>>> - Drop duplicate #include <linux/dmi.h> (already there at the end) >>>> --- >>>> drivers/mmc/host/sdhci-pci-core.c | 67 +++++++++++++++++++++++++++++++ >>>> drivers/mmc/host/sdhci-pci.h | 1 + >>>> 2 files changed, 68 insertions(+) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c >>>> index ed45ed0bdafd..a2ddbe3d8742 100644 >>>> --- a/drivers/mmc/host/sdhci-pci-core.c >>>> +++ b/drivers/mmc/host/sdhci-pci-core.c >>>> @@ -21,6 +21,7 @@ >>>> #include <linux/io.h> >>>> #include <linux/iopoll.h> >>>> #include <linux/gpio.h> >>>> +#include <linux/gpio/machine.h> >>>> #include <linux/pm_runtime.h> >>>> #include <linux/pm_qos.h> >>>> #include <linux/debugfs.h> >>>> @@ -1235,6 +1236,29 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = { >>>> .priv_size = sizeof(struct intel_host), >>>> }; >>>> >>>> +/* DMI quirks for devices with missing or broken CD GPIO info */ >>>> +static const struct gpiod_lookup_table vexia_edu_atla10_cd_gpios = { >>>> + .dev_id = "0000:00:12.0", >>>> + .table = { >>>> + GPIO_LOOKUP("INT33FC:00", 38, "cd", GPIO_ACTIVE_HIGH), >>>> + { } >>>> + }, >>>> +}; >>> >>> This is good but I feel like we should make it more difficult >>> to get the size wrong. Could introduce another struct to hold >>> the size: >>> >>> struct sdhci_pci_gpio_data { >>> const struct gpiod_lookup_table *gpios; >>> size_t size; >>> }; >>> >>> static const struct sdhci_pci_gpio_data vexia_edu_atla10_cd_gpio_data = { >>> .gpios = &vexia_edu_atla10_cd_gpios, >>> .size = sizeof(vexia_edu_atla10_cd_gpios), >>> }; >>> >>> So: >>> .driver_data = (void *)&vexia_edu_atla10_cd_gpio_data, >>> and >>> struct sdhci_pci_gpio_data *data; >>> ... >>> data = dmi_id->driver_data; >>> >>> cd_gpio_lookup_table = kmemdup(data->gpios, data->size, GFP_KERNEL); >> >> Interesting idea. But I'm afraid that sizeof(variable-name) on a struct >> with a flexible array member returns the same as just sizeof(struct struct-name) >> I added the following debug print to verify this: >> >> static int byt_sd_probe_slot(struct sdhci_pci_slot *slot) >> { >> + pr_info("sizeof(vexia_edu_atla10_cd_gpios) %lu sizeof(struct gpiod_lookup_table) %lu\n", >> + sizeof(vexia_edu_atla10_cd_gpios), sizeof(struct gpiod_lookup_table)); >> byt_probe_slot(slot); >> slot->host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | >> MMC_CAP_AGGRESSIVE_PM | MMC_CAP_CD_WAKE; >> >> And that prints: >> >> [ 10.459681] sizeof(vexia_edu_atla10_cd_gpios) 24 sizeof(struct gpiod_lookup_table) 24 >> >> So using sizeof(vexia_edu_atla10_cd_gpios) to get the size including the 2 >> flexible array members does not work since sizeof() does not take into >> account the size of the flexible array members. > > Thanks for spotting that! > > Perhaps we should check the table size then? > e.g. > struct gpiod_lookup_table *table; > size_t count; > > ... > > table = dmi_id->driver_data; > for (count = 0; table->table[count].key; count++) > ; > if (count != 1) > return ERR_PTR(-EINVAL); That works for me, but why not just use the found count instead of returning -EINVAL ? Regards, Hans > >>> >>>> + >>>> +static const struct dmi_system_id sdhci_intel_byt_cd_gpio_override[] = { >>>> + { >>>> + /* Vexia Edu Atla 10 tablet 9V version */ >>>> + .matches = { >>>> + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), >>>> + DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"), >>>> + /* Above strings are too generic, also match on BIOS date */ >>>> + DMI_MATCH(DMI_BIOS_DATE, "08/25/2014"), >>>> + }, >>>> + .driver_data = (void *)&vexia_edu_atla10_cd_gpios, >>>> + }, >>>> + { } >>>> +}; >>>> + >>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>>> #ifdef CONFIG_PM_SLEEP >>>> .resume = byt_resume, >>>> @@ -1253,6 +1277,7 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>>> .add_host = byt_add_host, >>>> .remove_slot = byt_remove_slot, >>>> .ops = &sdhci_intel_byt_ops, >>>> + .cd_gpio_override = sdhci_intel_byt_cd_gpio_override, >>>> .priv_size = sizeof(struct intel_host), >>>> }; >>>> >>>> @@ -2054,6 +2079,37 @@ static const struct dev_pm_ops sdhci_pci_pm_ops = { >>>> * * >>>> \*****************************************************************************/ >>>> >>>> +static struct gpiod_lookup_table *sdhci_pci_add_gpio_lookup_table( >>>> + struct sdhci_pci_chip *chip) >>> >>> Let's not line wrap until 100 columns >>> >>>> +{ >>>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>>> + const struct dmi_system_id *dmi_id = NULL; >>>> + >>>> + if (chip->fixes && chip->fixes->cd_gpio_override) >>>> + dmi_id = dmi_first_match(chip->fixes->cd_gpio_override); >>>> + >>>> + if (!dmi_id) >>>> + return NULL; >>>> + >>>> + cd_gpio_lookup_table = kmemdup(dmi_id->driver_data, >>>> + /* 1 GPIO lookup entry + 1 terminating entry */ >>>> + struct_size(cd_gpio_lookup_table, table, 2), >>>> + GFP_KERNEL); >>>> + if (!cd_gpio_lookup_table) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + gpiod_add_lookup_table(cd_gpio_lookup_table); >>>> + return cd_gpio_lookup_table; >>>> +} >>>> + >>>> +static void sdhci_pci_remove_gpio_lookup_table(struct gpiod_lookup_table *lookup_table) >>>> +{ >>>> + if (lookup_table) { >>>> + gpiod_remove_lookup_table(lookup_table); >>>> + kfree(lookup_table); >>>> + } >>>> +} >>>> + >>>> static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>>> struct pci_dev *pdev, struct sdhci_pci_chip *chip, int first_bar, >>>> int slotno) >>>> @@ -2129,8 +2185,19 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>>> device_init_wakeup(&pdev->dev, true); >>>> >>>> if (slot->cd_idx >= 0) { >>>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>>> + >>>> + cd_gpio_lookup_table = sdhci_pci_add_gpio_lookup_table(chip); >>>> + if (IS_ERR(cd_gpio_lookup_table)) { >>>> + ret = PTR_ERR(cd_gpio_lookup_table); >>>> + goto remove; >>>> + } >>>> + >>>> ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, >>>> slot->cd_override_level, 0); >>>> + >>>> + sdhci_pci_remove_gpio_lookup_table(cd_gpio_lookup_table); >>>> + >>>> if (ret && ret != -EPROBE_DEFER) >>>> ret = mmc_gpiod_request_cd(host->mmc, NULL, >>>> slot->cd_idx, >>>> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h >>>> index 153704f812ed..4973fa859217 100644 >>>> --- a/drivers/mmc/host/sdhci-pci.h >>>> +++ b/drivers/mmc/host/sdhci-pci.h >>>> @@ -156,6 +156,7 @@ struct sdhci_pci_fixes { >>>> #endif >>>> >>>> const struct sdhci_ops *ops; >>>> + const struct dmi_system_id *cd_gpio_override; >>>> size_t priv_size; >>>> }; >>>> >>> >> >
On 18/11/24 13:53, Hans de Goede wrote: > Hi, > > On 18-Nov-24 11:10 AM, Adrian Hunter wrote: >> On 16/11/24 13:25, Hans de Goede wrote: >>> Hi Adrian, >>> >>> On 15-Nov-24 8:33 AM, Adrian Hunter wrote: >>>> On 14/11/24 17:56, Hans de Goede wrote: >>>>> The Vexia Edu Atla 10 tablet distributed to schools in the Spanish >>>>> Andalucía region has no ACPI fwnode associated with the SDHCI controller >>>>> for its microsd-slot and thus has no ACPI GPIO resource info. >>>>> >>>>> This causes the following error to be logged and the slot to not work: >>>>> [ 10.572113] sdhci-pci 0000:00:12.0: failed to setup card detect gpio >>>>> >>>>> Add a DMI quirk table for providing gpiod_lookup_tables with manually >>>>> provided CD GPIO info and use this DMI table to provide the CD GPIO info >>>>> on this tablet. This fixes the microsd-slot not working. >>>>> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> Changes in v3: >>>>> - Add a cd_gpio_override pointer to sdhci_pci_fixes >>>>> - Add sdhci_pci_add_gpio_lookup_table() helper which kmemdup-s a const >>>>> struct gpiod_lookup_table to avoid races when using async probing >>>>> >>>>> Changes in v2: >>>>> - Make sdhci_pci_dmi_cd_gpio_overrides static const instead of just const >>>>> - Drop duplicate #include <linux/dmi.h> (already there at the end) >>>>> --- >>>>> drivers/mmc/host/sdhci-pci-core.c | 67 +++++++++++++++++++++++++++++++ >>>>> drivers/mmc/host/sdhci-pci.h | 1 + >>>>> 2 files changed, 68 insertions(+) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c >>>>> index ed45ed0bdafd..a2ddbe3d8742 100644 >>>>> --- a/drivers/mmc/host/sdhci-pci-core.c >>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c >>>>> @@ -21,6 +21,7 @@ >>>>> #include <linux/io.h> >>>>> #include <linux/iopoll.h> >>>>> #include <linux/gpio.h> >>>>> +#include <linux/gpio/machine.h> >>>>> #include <linux/pm_runtime.h> >>>>> #include <linux/pm_qos.h> >>>>> #include <linux/debugfs.h> >>>>> @@ -1235,6 +1236,29 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = { >>>>> .priv_size = sizeof(struct intel_host), >>>>> }; >>>>> >>>>> +/* DMI quirks for devices with missing or broken CD GPIO info */ >>>>> +static const struct gpiod_lookup_table vexia_edu_atla10_cd_gpios = { >>>>> + .dev_id = "0000:00:12.0", >>>>> + .table = { >>>>> + GPIO_LOOKUP("INT33FC:00", 38, "cd", GPIO_ACTIVE_HIGH), >>>>> + { } >>>>> + }, >>>>> +}; >>>> >>>> This is good but I feel like we should make it more difficult >>>> to get the size wrong. Could introduce another struct to hold >>>> the size: >>>> >>>> struct sdhci_pci_gpio_data { >>>> const struct gpiod_lookup_table *gpios; >>>> size_t size; >>>> }; >>>> >>>> static const struct sdhci_pci_gpio_data vexia_edu_atla10_cd_gpio_data = { >>>> .gpios = &vexia_edu_atla10_cd_gpios, >>>> .size = sizeof(vexia_edu_atla10_cd_gpios), >>>> }; >>>> >>>> So: >>>> .driver_data = (void *)&vexia_edu_atla10_cd_gpio_data, >>>> and >>>> struct sdhci_pci_gpio_data *data; >>>> ... >>>> data = dmi_id->driver_data; >>>> >>>> cd_gpio_lookup_table = kmemdup(data->gpios, data->size, GFP_KERNEL); >>> >>> Interesting idea. But I'm afraid that sizeof(variable-name) on a struct >>> with a flexible array member returns the same as just sizeof(struct struct-name) >>> I added the following debug print to verify this: >>> >>> static int byt_sd_probe_slot(struct sdhci_pci_slot *slot) >>> { >>> + pr_info("sizeof(vexia_edu_atla10_cd_gpios) %lu sizeof(struct gpiod_lookup_table) %lu\n", >>> + sizeof(vexia_edu_atla10_cd_gpios), sizeof(struct gpiod_lookup_table)); >>> byt_probe_slot(slot); >>> slot->host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | >>> MMC_CAP_AGGRESSIVE_PM | MMC_CAP_CD_WAKE; >>> >>> And that prints: >>> >>> [ 10.459681] sizeof(vexia_edu_atla10_cd_gpios) 24 sizeof(struct gpiod_lookup_table) 24 >>> >>> So using sizeof(vexia_edu_atla10_cd_gpios) to get the size including the 2 >>> flexible array members does not work since sizeof() does not take into >>> account the size of the flexible array members. >> >> Thanks for spotting that! >> >> Perhaps we should check the table size then? >> e.g. >> struct gpiod_lookup_table *table; >> size_t count; >> >> ... >> >> table = dmi_id->driver_data; >> for (count = 0; table->table[count].key; count++) >> ; >> if (count != 1) >> return ERR_PTR(-EINVAL); > > That works for me, but why not just use the found count instead of > returning -EINVAL ? I was thinking it avoids the count == 0 case, but I guess it doesn't actually matter. The kmemdup size would need to use count + 1 > > Regards, > > Hans > > > >> >>>> >>>>> + >>>>> +static const struct dmi_system_id sdhci_intel_byt_cd_gpio_override[] = { >>>>> + { >>>>> + /* Vexia Edu Atla 10 tablet 9V version */ >>>>> + .matches = { >>>>> + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), >>>>> + DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"), >>>>> + /* Above strings are too generic, also match on BIOS date */ >>>>> + DMI_MATCH(DMI_BIOS_DATE, "08/25/2014"), >>>>> + }, >>>>> + .driver_data = (void *)&vexia_edu_atla10_cd_gpios, >>>>> + }, >>>>> + { } >>>>> +}; >>>>> + >>>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>>>> #ifdef CONFIG_PM_SLEEP >>>>> .resume = byt_resume, >>>>> @@ -1253,6 +1277,7 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>>>> .add_host = byt_add_host, >>>>> .remove_slot = byt_remove_slot, >>>>> .ops = &sdhci_intel_byt_ops, >>>>> + .cd_gpio_override = sdhci_intel_byt_cd_gpio_override, >>>>> .priv_size = sizeof(struct intel_host), >>>>> }; >>>>> >>>>> @@ -2054,6 +2079,37 @@ static const struct dev_pm_ops sdhci_pci_pm_ops = { >>>>> * * >>>>> \*****************************************************************************/ >>>>> >>>>> +static struct gpiod_lookup_table *sdhci_pci_add_gpio_lookup_table( >>>>> + struct sdhci_pci_chip *chip) >>>> >>>> Let's not line wrap until 100 columns >>>> >>>>> +{ >>>>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>>>> + const struct dmi_system_id *dmi_id = NULL; >>>>> + >>>>> + if (chip->fixes && chip->fixes->cd_gpio_override) >>>>> + dmi_id = dmi_first_match(chip->fixes->cd_gpio_override); >>>>> + >>>>> + if (!dmi_id) >>>>> + return NULL; >>>>> + >>>>> + cd_gpio_lookup_table = kmemdup(dmi_id->driver_data, >>>>> + /* 1 GPIO lookup entry + 1 terminating entry */ >>>>> + struct_size(cd_gpio_lookup_table, table, 2), >>>>> + GFP_KERNEL); >>>>> + if (!cd_gpio_lookup_table) >>>>> + return ERR_PTR(-ENOMEM); >>>>> + >>>>> + gpiod_add_lookup_table(cd_gpio_lookup_table); >>>>> + return cd_gpio_lookup_table; >>>>> +} >>>>> + >>>>> +static void sdhci_pci_remove_gpio_lookup_table(struct gpiod_lookup_table *lookup_table) >>>>> +{ >>>>> + if (lookup_table) { >>>>> + gpiod_remove_lookup_table(lookup_table); >>>>> + kfree(lookup_table); >>>>> + } >>>>> +} >>>>> + >>>>> static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>>>> struct pci_dev *pdev, struct sdhci_pci_chip *chip, int first_bar, >>>>> int slotno) >>>>> @@ -2129,8 +2185,19 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>>>> device_init_wakeup(&pdev->dev, true); >>>>> >>>>> if (slot->cd_idx >= 0) { >>>>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>>>> + >>>>> + cd_gpio_lookup_table = sdhci_pci_add_gpio_lookup_table(chip); >>>>> + if (IS_ERR(cd_gpio_lookup_table)) { >>>>> + ret = PTR_ERR(cd_gpio_lookup_table); >>>>> + goto remove; >>>>> + } >>>>> + >>>>> ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, >>>>> slot->cd_override_level, 0); >>>>> + >>>>> + sdhci_pci_remove_gpio_lookup_table(cd_gpio_lookup_table); >>>>> + >>>>> if (ret && ret != -EPROBE_DEFER) >>>>> ret = mmc_gpiod_request_cd(host->mmc, NULL, >>>>> slot->cd_idx, >>>>> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h >>>>> index 153704f812ed..4973fa859217 100644 >>>>> --- a/drivers/mmc/host/sdhci-pci.h >>>>> +++ b/drivers/mmc/host/sdhci-pci.h >>>>> @@ -156,6 +156,7 @@ struct sdhci_pci_fixes { >>>>> #endif >>>>> >>>>> const struct sdhci_ops *ops; >>>>> + const struct dmi_system_id *cd_gpio_override; >>>>> size_t priv_size; >>>>> }; >>>>> >>>> >>> >> >
Hi, On 18-Nov-24 1:11 PM, Adrian Hunter wrote: > On 18/11/24 13:53, Hans de Goede wrote: >> Hi, >> >> On 18-Nov-24 11:10 AM, Adrian Hunter wrote: >>> On 16/11/24 13:25, Hans de Goede wrote: >>>> Hi Adrian, >>>> >>>> On 15-Nov-24 8:33 AM, Adrian Hunter wrote: >>>>> On 14/11/24 17:56, Hans de Goede wrote: >>>>>> The Vexia Edu Atla 10 tablet distributed to schools in the Spanish >>>>>> Andalucía region has no ACPI fwnode associated with the SDHCI controller >>>>>> for its microsd-slot and thus has no ACPI GPIO resource info. >>>>>> >>>>>> This causes the following error to be logged and the slot to not work: >>>>>> [ 10.572113] sdhci-pci 0000:00:12.0: failed to setup card detect gpio >>>>>> >>>>>> Add a DMI quirk table for providing gpiod_lookup_tables with manually >>>>>> provided CD GPIO info and use this DMI table to provide the CD GPIO info >>>>>> on this tablet. This fixes the microsd-slot not working. >>>>>> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>> --- >>>>>> Changes in v3: >>>>>> - Add a cd_gpio_override pointer to sdhci_pci_fixes >>>>>> - Add sdhci_pci_add_gpio_lookup_table() helper which kmemdup-s a const >>>>>> struct gpiod_lookup_table to avoid races when using async probing >>>>>> >>>>>> Changes in v2: >>>>>> - Make sdhci_pci_dmi_cd_gpio_overrides static const instead of just const >>>>>> - Drop duplicate #include <linux/dmi.h> (already there at the end) >>>>>> --- >>>>>> drivers/mmc/host/sdhci-pci-core.c | 67 +++++++++++++++++++++++++++++++ >>>>>> drivers/mmc/host/sdhci-pci.h | 1 + >>>>>> 2 files changed, 68 insertions(+) >>>>>> >>>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c >>>>>> index ed45ed0bdafd..a2ddbe3d8742 100644 >>>>>> --- a/drivers/mmc/host/sdhci-pci-core.c >>>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c >>>>>> @@ -21,6 +21,7 @@ >>>>>> #include <linux/io.h> >>>>>> #include <linux/iopoll.h> >>>>>> #include <linux/gpio.h> >>>>>> +#include <linux/gpio/machine.h> >>>>>> #include <linux/pm_runtime.h> >>>>>> #include <linux/pm_qos.h> >>>>>> #include <linux/debugfs.h> >>>>>> @@ -1235,6 +1236,29 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = { >>>>>> .priv_size = sizeof(struct intel_host), >>>>>> }; >>>>>> >>>>>> +/* DMI quirks for devices with missing or broken CD GPIO info */ >>>>>> +static const struct gpiod_lookup_table vexia_edu_atla10_cd_gpios = { >>>>>> + .dev_id = "0000:00:12.0", >>>>>> + .table = { >>>>>> + GPIO_LOOKUP("INT33FC:00", 38, "cd", GPIO_ACTIVE_HIGH), >>>>>> + { } >>>>>> + }, >>>>>> +}; >>>>> >>>>> This is good but I feel like we should make it more difficult >>>>> to get the size wrong. Could introduce another struct to hold >>>>> the size: >>>>> >>>>> struct sdhci_pci_gpio_data { >>>>> const struct gpiod_lookup_table *gpios; >>>>> size_t size; >>>>> }; >>>>> >>>>> static const struct sdhci_pci_gpio_data vexia_edu_atla10_cd_gpio_data = { >>>>> .gpios = &vexia_edu_atla10_cd_gpios, >>>>> .size = sizeof(vexia_edu_atla10_cd_gpios), >>>>> }; >>>>> >>>>> So: >>>>> .driver_data = (void *)&vexia_edu_atla10_cd_gpio_data, >>>>> and >>>>> struct sdhci_pci_gpio_data *data; >>>>> ... >>>>> data = dmi_id->driver_data; >>>>> >>>>> cd_gpio_lookup_table = kmemdup(data->gpios, data->size, GFP_KERNEL); >>>> >>>> Interesting idea. But I'm afraid that sizeof(variable-name) on a struct >>>> with a flexible array member returns the same as just sizeof(struct struct-name) >>>> I added the following debug print to verify this: >>>> >>>> static int byt_sd_probe_slot(struct sdhci_pci_slot *slot) >>>> { >>>> + pr_info("sizeof(vexia_edu_atla10_cd_gpios) %lu sizeof(struct gpiod_lookup_table) %lu\n", >>>> + sizeof(vexia_edu_atla10_cd_gpios), sizeof(struct gpiod_lookup_table)); >>>> byt_probe_slot(slot); >>>> slot->host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | >>>> MMC_CAP_AGGRESSIVE_PM | MMC_CAP_CD_WAKE; >>>> >>>> And that prints: >>>> >>>> [ 10.459681] sizeof(vexia_edu_atla10_cd_gpios) 24 sizeof(struct gpiod_lookup_table) 24 >>>> >>>> So using sizeof(vexia_edu_atla10_cd_gpios) to get the size including the 2 >>>> flexible array members does not work since sizeof() does not take into >>>> account the size of the flexible array members. >>> >>> Thanks for spotting that! >>> >>> Perhaps we should check the table size then? >>> e.g. >>> struct gpiod_lookup_table *table; >>> size_t count; >>> >>> ... >>> >>> table = dmi_id->driver_data; >>> for (count = 0; table->table[count].key; count++) >>> ; >>> if (count != 1) >>> return ERR_PTR(-EINVAL); >> >> That works for me, but why not just use the found count instead of >> returning -EINVAL ? > > I was thinking it avoids the count == 0 case, but I guess it > doesn't actually matter. > > The kmemdup size would need to use count + 1 Ack, I will prepare a v4 with this added. Regards, Hans >>>>>> + >>>>>> +static const struct dmi_system_id sdhci_intel_byt_cd_gpio_override[] = { >>>>>> + { >>>>>> + /* Vexia Edu Atla 10 tablet 9V version */ >>>>>> + .matches = { >>>>>> + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), >>>>>> + DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"), >>>>>> + /* Above strings are too generic, also match on BIOS date */ >>>>>> + DMI_MATCH(DMI_BIOS_DATE, "08/25/2014"), >>>>>> + }, >>>>>> + .driver_data = (void *)&vexia_edu_atla10_cd_gpios, >>>>>> + }, >>>>>> + { } >>>>>> +}; >>>>>> + >>>>>> static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>>>>> #ifdef CONFIG_PM_SLEEP >>>>>> .resume = byt_resume, >>>>>> @@ -1253,6 +1277,7 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { >>>>>> .add_host = byt_add_host, >>>>>> .remove_slot = byt_remove_slot, >>>>>> .ops = &sdhci_intel_byt_ops, >>>>>> + .cd_gpio_override = sdhci_intel_byt_cd_gpio_override, >>>>>> .priv_size = sizeof(struct intel_host), >>>>>> }; >>>>>> >>>>>> @@ -2054,6 +2079,37 @@ static const struct dev_pm_ops sdhci_pci_pm_ops = { >>>>>> * * >>>>>> \*****************************************************************************/ >>>>>> >>>>>> +static struct gpiod_lookup_table *sdhci_pci_add_gpio_lookup_table( >>>>>> + struct sdhci_pci_chip *chip) >>>>> >>>>> Let's not line wrap until 100 columns >>>>> >>>>>> +{ >>>>>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>>>>> + const struct dmi_system_id *dmi_id = NULL; >>>>>> + >>>>>> + if (chip->fixes && chip->fixes->cd_gpio_override) >>>>>> + dmi_id = dmi_first_match(chip->fixes->cd_gpio_override); >>>>>> + >>>>>> + if (!dmi_id) >>>>>> + return NULL; >>>>>> + >>>>>> + cd_gpio_lookup_table = kmemdup(dmi_id->driver_data, >>>>>> + /* 1 GPIO lookup entry + 1 terminating entry */ >>>>>> + struct_size(cd_gpio_lookup_table, table, 2), >>>>>> + GFP_KERNEL); >>>>>> + if (!cd_gpio_lookup_table) >>>>>> + return ERR_PTR(-ENOMEM); >>>>>> + >>>>>> + gpiod_add_lookup_table(cd_gpio_lookup_table); >>>>>> + return cd_gpio_lookup_table; >>>>>> +} >>>>>> + >>>>>> +static void sdhci_pci_remove_gpio_lookup_table(struct gpiod_lookup_table *lookup_table) >>>>>> +{ >>>>>> + if (lookup_table) { >>>>>> + gpiod_remove_lookup_table(lookup_table); >>>>>> + kfree(lookup_table); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>>>>> struct pci_dev *pdev, struct sdhci_pci_chip *chip, int first_bar, >>>>>> int slotno) >>>>>> @@ -2129,8 +2185,19 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( >>>>>> device_init_wakeup(&pdev->dev, true); >>>>>> >>>>>> if (slot->cd_idx >= 0) { >>>>>> + struct gpiod_lookup_table *cd_gpio_lookup_table; >>>>>> + >>>>>> + cd_gpio_lookup_table = sdhci_pci_add_gpio_lookup_table(chip); >>>>>> + if (IS_ERR(cd_gpio_lookup_table)) { >>>>>> + ret = PTR_ERR(cd_gpio_lookup_table); >>>>>> + goto remove; >>>>>> + } >>>>>> + >>>>>> ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, >>>>>> slot->cd_override_level, 0); >>>>>> + >>>>>> + sdhci_pci_remove_gpio_lookup_table(cd_gpio_lookup_table); >>>>>> + >>>>>> if (ret && ret != -EPROBE_DEFER) >>>>>> ret = mmc_gpiod_request_cd(host->mmc, NULL, >>>>>> slot->cd_idx, >>>>>> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h >>>>>> index 153704f812ed..4973fa859217 100644 >>>>>> --- a/drivers/mmc/host/sdhci-pci.h >>>>>> +++ b/drivers/mmc/host/sdhci-pci.h >>>>>> @@ -156,6 +156,7 @@ struct sdhci_pci_fixes { >>>>>> #endif >>>>>> >>>>>> const struct sdhci_ops *ops; >>>>>> + const struct dmi_system_id *cd_gpio_override; >>>>>> size_t priv_size; >>>>>> }; >>>>>> >>>>> >>>> >>> >> >
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c index ed45ed0bdafd..a2ddbe3d8742 100644 --- a/drivers/mmc/host/sdhci-pci-core.c +++ b/drivers/mmc/host/sdhci-pci-core.c @@ -21,6 +21,7 @@ #include <linux/io.h> #include <linux/iopoll.h> #include <linux/gpio.h> +#include <linux/gpio/machine.h> #include <linux/pm_runtime.h> #include <linux/pm_qos.h> #include <linux/debugfs.h> @@ -1235,6 +1236,29 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sdio = { .priv_size = sizeof(struct intel_host), }; +/* DMI quirks for devices with missing or broken CD GPIO info */ +static const struct gpiod_lookup_table vexia_edu_atla10_cd_gpios = { + .dev_id = "0000:00:12.0", + .table = { + GPIO_LOOKUP("INT33FC:00", 38, "cd", GPIO_ACTIVE_HIGH), + { } + }, +}; + +static const struct dmi_system_id sdhci_intel_byt_cd_gpio_override[] = { + { + /* Vexia Edu Atla 10 tablet 9V version */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"), + /* Above strings are too generic, also match on BIOS date */ + DMI_MATCH(DMI_BIOS_DATE, "08/25/2014"), + }, + .driver_data = (void *)&vexia_edu_atla10_cd_gpios, + }, + { } +}; + static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { #ifdef CONFIG_PM_SLEEP .resume = byt_resume, @@ -1253,6 +1277,7 @@ static const struct sdhci_pci_fixes sdhci_intel_byt_sd = { .add_host = byt_add_host, .remove_slot = byt_remove_slot, .ops = &sdhci_intel_byt_ops, + .cd_gpio_override = sdhci_intel_byt_cd_gpio_override, .priv_size = sizeof(struct intel_host), }; @@ -2054,6 +2079,37 @@ static const struct dev_pm_ops sdhci_pci_pm_ops = { * * \*****************************************************************************/ +static struct gpiod_lookup_table *sdhci_pci_add_gpio_lookup_table( + struct sdhci_pci_chip *chip) +{ + struct gpiod_lookup_table *cd_gpio_lookup_table; + const struct dmi_system_id *dmi_id = NULL; + + if (chip->fixes && chip->fixes->cd_gpio_override) + dmi_id = dmi_first_match(chip->fixes->cd_gpio_override); + + if (!dmi_id) + return NULL; + + cd_gpio_lookup_table = kmemdup(dmi_id->driver_data, + /* 1 GPIO lookup entry + 1 terminating entry */ + struct_size(cd_gpio_lookup_table, table, 2), + GFP_KERNEL); + if (!cd_gpio_lookup_table) + return ERR_PTR(-ENOMEM); + + gpiod_add_lookup_table(cd_gpio_lookup_table); + return cd_gpio_lookup_table; +} + +static void sdhci_pci_remove_gpio_lookup_table(struct gpiod_lookup_table *lookup_table) +{ + if (lookup_table) { + gpiod_remove_lookup_table(lookup_table); + kfree(lookup_table); + } +} + static struct sdhci_pci_slot *sdhci_pci_probe_slot( struct pci_dev *pdev, struct sdhci_pci_chip *chip, int first_bar, int slotno) @@ -2129,8 +2185,19 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( device_init_wakeup(&pdev->dev, true); if (slot->cd_idx >= 0) { + struct gpiod_lookup_table *cd_gpio_lookup_table; + + cd_gpio_lookup_table = sdhci_pci_add_gpio_lookup_table(chip); + if (IS_ERR(cd_gpio_lookup_table)) { + ret = PTR_ERR(cd_gpio_lookup_table); + goto remove; + } + ret = mmc_gpiod_request_cd(host->mmc, "cd", slot->cd_idx, slot->cd_override_level, 0); + + sdhci_pci_remove_gpio_lookup_table(cd_gpio_lookup_table); + if (ret && ret != -EPROBE_DEFER) ret = mmc_gpiod_request_cd(host->mmc, NULL, slot->cd_idx, diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h index 153704f812ed..4973fa859217 100644 --- a/drivers/mmc/host/sdhci-pci.h +++ b/drivers/mmc/host/sdhci-pci.h @@ -156,6 +156,7 @@ struct sdhci_pci_fixes { #endif const struct sdhci_ops *ops; + const struct dmi_system_id *cd_gpio_override; size_t priv_size; };
The Vexia Edu Atla 10 tablet distributed to schools in the Spanish Andalucía region has no ACPI fwnode associated with the SDHCI controller for its microsd-slot and thus has no ACPI GPIO resource info. This causes the following error to be logged and the slot to not work: [ 10.572113] sdhci-pci 0000:00:12.0: failed to setup card detect gpio Add a DMI quirk table for providing gpiod_lookup_tables with manually provided CD GPIO info and use this DMI table to provide the CD GPIO info on this tablet. This fixes the microsd-slot not working. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v3: - Add a cd_gpio_override pointer to sdhci_pci_fixes - Add sdhci_pci_add_gpio_lookup_table() helper which kmemdup-s a const struct gpiod_lookup_table to avoid races when using async probing Changes in v2: - Make sdhci_pci_dmi_cd_gpio_overrides static const instead of just const - Drop duplicate #include <linux/dmi.h> (already there at the end) --- drivers/mmc/host/sdhci-pci-core.c | 67 +++++++++++++++++++++++++++++++ drivers/mmc/host/sdhci-pci.h | 1 + 2 files changed, 68 insertions(+)