[PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[Hans de Goede]

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),
+		{ }
+	},
+};
+
+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;
 };
 
-- 
2.47.0

Re: [PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[Adrian Hunter]

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;
>  };
>  

Re: [PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[Hans de Goede]

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;
>>  };
>>  
> 

Re: [PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[Adrian Hunter]

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;
>>>  };
>>>  
>>
> 

Re: [PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[Hans de Goede]

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;
>>>>  };
>>>>  
>>>
>>
> 

Re: [PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[Adrian Hunter]

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;
>>>>>  };
>>>>>  
>>>>
>>>
>>
> 

Re: [PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet

[Hans de Goede]

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;
>>>>>>  };
>>>>>>  
>>>>>
>>>>
>>>
>>
>