From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 61D2B13A89A for ; Mon, 18 Nov 2024 12:12:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731931938; cv=none; b=fEHrBQfd/13Pxi6wVEl2k4YnNQv81pJJzNUO+wWuzN3iS3XAyE5s8dHD3et8iwLEq1jodCSf+/6HmBKrHeLFHTokRvfYA6RC9vXC1nDt18eP64LCXDN8fnoEkk81pfBBDLLwO5b16goZSznuRPGIGkWTEC004Wf60Y5lpRBQtzs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731931938; c=relaxed/simple; bh=m8iDYtRfOvxnPwltCkLGucmJ/R0pauK5+UKZr8nLfuk=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=nxxnAk8i7PdTsmNqV/KSi3iwZ317AbBxxNHEVncZUE94RK1I33mSUjAtGtRfvy3xBa8Qd+ossdaRK1rBhHKqv4w1OIJeAcCvu4T6LWhVJeRgpmFCxVPIIJlDMUszj7azotEnl8A4P758p2kLmwIR+b+c79D6C1dDk7qVVj7V6p4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=Ft0MhnAC; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="Ft0MhnAC" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1731931935; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=H0YNyjR+eEC7BfBax8B900y4I+1mklNkSCUtP77YhiY=; b=Ft0MhnAC0EtvkevpxixhG0RDRpbkojOxyOHbAYgZdodosr3akvPjxPX5ZQSbVSUArUauOt wY6LlnF3urusXnFBApFuFc6XHN+j/IAL6U+AD1pEPgFesIoIcc79YMI/plVDMXMtq/wjbG 6BBl36RJeOTRiPHj1dUHQlneXK94/TI= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-617-SyLc2JrjNB6-jjWKHc4t5w-1; Mon, 18 Nov 2024 07:12:14 -0500 X-MC-Unique: SyLc2JrjNB6-jjWKHc4t5w-1 X-Mimecast-MFC-AGG-ID: SyLc2JrjNB6-jjWKHc4t5w Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-aa2099d0114so468199766b.1 for ; Mon, 18 Nov 2024 04:12:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731931933; x=1732536733; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=H0YNyjR+eEC7BfBax8B900y4I+1mklNkSCUtP77YhiY=; b=IwUum9rb6HKGngUvFty1ivOjOfbtg8r4Dqr7gMuFXAlV2m7WmeiuQribH28MtR+y2f vu58uDJR4wFODkWm52p0Xidx4Y4vQ8jQQKksyk6cyqyGG+KA/m2lcOdUvZqLzbgiFL38 pFfsx3mmKnPGmT5nAuCHe/Qqo4+1dAsHF86MEsQPEbg5ni2v+dQPJwWPGmHzQtEa3kP0 TyJLmEsn4ilRx5bLSUZxek7aafxSY5xlbRJwVN9R0C9xhbDgI+RRENCyiQahh4IBMkGu 8ReLce6lA6R967OyNSq4fJFPG+M7qBBcX7eKjPg7svRUooiSeYuirch2Ibwe0O2UVsmq 5A2g== X-Gm-Message-State: AOJu0YyZAn+Gbif1Y14brzMKdb56VGC4aO38VuLJQ0BMzurKayPl+ld8 +Kk3GndFk8+59leUkJ4t3mhYhbWQ3GTOrYvFVCsRqw+cg/bxCgZIA8Ln2UnP69fJfVlH0qWl3bG E5pP8zooMJszqOcgKALYsLmvoLRpb7atspAa2YL8sm+lsRhuQ0/9FemdXAe3lk4A17w== X-Received: by 2002:a17:907:2d20:b0:a99:f94b:f92e with SMTP id a640c23a62f3a-aa2078168e5mr1351665266b.27.1731931932748; Mon, 18 Nov 2024 04:12:12 -0800 (PST) X-Google-Smtp-Source: AGHT+IGVbcA1FF+oywA4SB6yF5FlXVlGdObX240M902oUJkcu14ZdFw1+XR72L3GS2H+e0l2aL9E4Q== X-Received: by 2002:a17:907:2d20:b0:a99:f94b:f92e with SMTP id a640c23a62f3a-aa2078168e5mr1351662266b.27.1731931932315; Mon, 18 Nov 2024 04:12:12 -0800 (PST) Received: from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec? (2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl. [2001:1c00:c32:7800:5bfa:a036:83f0:f9ec]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-aa20e046ea8sm533856866b.169.2024.11.18.04.12.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 18 Nov 2024 04:12:11 -0800 (PST) Message-ID: Date: Mon, 18 Nov 2024 13:12:11 +0100 Precedence: bulk X-Mailing-List: linux-mmc@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] mmc: sdhci-pci: Add DMI quirk for missing CD GPIO on Vexia Edu Atla 10 tablet To: Adrian Hunter , Ulf Hansson Cc: linux-mmc@vger.kernel.org References: <20241114155611.59577-1-hdegoede@redhat.com> <2008ebd5-b1d1-417e-bd88-9d4c0a381e7d@intel.com> <1196b26c-e5d3-4f2d-a97b-688e9825c98a@redhat.com> <686a5c43-ac82-4506-ade8-e666a538abd5@intel.com> <5535807c-ce12-45a7-a7ea-44c03474e692@redhat.com> Content-Language: en-US, nl From: Hans de Goede In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 >>>>>> --- >>>>>> 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 (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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> #include >>>>>> #include >>>>>> #include >>>>>> @@ -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; >>>>>> }; >>>>>> >>>>> >>>> >>> >> >