[PATCH] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[PATCH] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Fernando Eckhardt Valle]

Newer Thinkpads have a feature called Mac Address Passthrough.
This patch provides a sysfs interface that userspace can use
to get this auxiliary mac address.

Signed-off-by: Fernando Eckhardt Valle <fevalle@ipt.br>
---
 drivers/platform/x86/thinkpad_acpi.c | 77 ++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index d70c89d32..0b1c36b0d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10785,6 +10785,78 @@ static struct ibm_struct dprc_driver_data = {
 	.name = "dprc",
 };
 
+/*
+ * Auxmac
+ *
+ * This auxiliary mac address is enabled in the bios through the
+ * Mac Address Passthrough feature. In most cases, there are three
+ * possibilities: Internal Mac, Second Mac, and disabled.
+ *
+ */
+
+#define AUXMAC_LEN 12
+#define AUXMAC_START 9
+#define AUXMAC_STRLEN 22
+static char auxmac[AUXMAC_LEN];
+
+static int auxmac_init(struct ibm_init_struct *iibm)
+{
+	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+
+	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
+
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	obj = (union acpi_object *)buffer.pointer;
+
+	if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
+		pr_info("Invalid buffer for mac addr passthrough.\n");
+		goto auxmacinvalid;
+	}
+
+	if (strncmp(obj->string.pointer + 0x8, "#", 1) != 0 ||
+	    strncmp(obj->string.pointer + 0x15, "#", 1) != 0) {
+		pr_info("Invalid header for mac addr passthrough.\n");
+		goto auxmacinvalid;
+	}
+
+	memcpy(auxmac, obj->string.pointer + AUXMAC_START, AUXMAC_LEN);
+	kfree(obj);
+	return 0;
+
+auxmacinvalid:
+	kfree(obj);
+	memcpy(auxmac, "unavailable", 11);
+	return 0;
+}
+
+static struct ibm_struct auxmac_data = {
+	.name = "auxmac",
+};
+
+static ssize_t auxmac_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	if (strncmp(auxmac, "XXXXXXXXXXXX", AUXMAC_LEN) == 0)
+		memcpy(auxmac, "disabled", 9);
+
+	return sysfs_emit(buf, "%s\n", auxmac);
+}
+static DEVICE_ATTR_RO(auxmac);
+
+static struct attribute *auxmac_attributes[] = {
+	&dev_attr_auxmac.attr,
+	NULL
+};
+
+static const struct attribute_group auxmac_attr_group = {
+	.attrs = auxmac_attributes,
+};
+
 /* --------------------------------------------------------------------- */
 
 static struct attribute *tpacpi_driver_attributes[] = {
@@ -10843,6 +10915,7 @@ static const struct attribute_group *tpacpi_groups[] = {
 	&proxsensor_attr_group,
 	&kbdlang_attr_group,
 	&dprc_attr_group,
+	&auxmac_attr_group,
 	NULL,
 };
 
@@ -11414,6 +11487,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = tpacpi_dprc_init,
 		.data = &dprc_driver_data,
 	},
+	{
+		.init = auxmac_init,
+		.data = &auxmac_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
-- 
2.25.1

Re: [PATCH] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Hans de Goede]

Hi Fernando,

On 9/6/23 21:52, Fernando Eckhardt Valle wrote:
> Newer Thinkpads have a feature called Mac Address Passthrough.
> This patch provides a sysfs interface that userspace can use
> to get this auxiliary mac address.
> 
> Signed-off-by: Fernando Eckhardt Valle <fevalle@ipt.br>

Thank you for your patch. 

At a minimum for this patch to be accepted you will need
to document the new sysfs interface in:

Documentation/admin-guide/laptops/thinkpad-acpi.rst

But I wonder if we should export this information to
userspace in this way ?

The reason why I'm wondering is because mac-address passthrough
in case of using e.g. Lenovo Thunderbolt docks is already
supported by the kernel by code for this in drivers/net/usb/r8152.c :

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/usb/r8152.c#n1613

So I'm wondering if we really need this, is there a planned
userspace API consumer of the new sysfs interface ?

Or is this only intended as a way for a user to query this, iow
is this purely intended for informational purposes ?

Regards,

Hans






> ---
>  drivers/platform/x86/thinkpad_acpi.c | 77 ++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d70c89d32..0b1c36b0d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10785,6 +10785,78 @@ static struct ibm_struct dprc_driver_data = {
>  	.name = "dprc",
>  };
>  
> +/*
> + * Auxmac
> + *
> + * This auxiliary mac address is enabled in the bios through the
> + * Mac Address Passthrough feature. In most cases, there are three
> + * possibilities: Internal Mac, Second Mac, and disabled.
> + *
> + */
> +
> +#define AUXMAC_LEN 12
> +#define AUXMAC_START 9
> +#define AUXMAC_STRLEN 22
> +static char auxmac[AUXMAC_LEN];
> +
> +static int auxmac_init(struct ibm_init_struct *iibm)
> +{
> +	acpi_status status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +
> +	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
> +
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	obj = (union acpi_object *)buffer.pointer;
> +
> +	if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
> +		pr_info("Invalid buffer for mac addr passthrough.\n");
> +		goto auxmacinvalid;
> +	}
> +
> +	if (strncmp(obj->string.pointer + 0x8, "#", 1) != 0 ||
> +	    strncmp(obj->string.pointer + 0x15, "#", 1) != 0) {
> +		pr_info("Invalid header for mac addr passthrough.\n");
> +		goto auxmacinvalid;
> +	}
> +
> +	memcpy(auxmac, obj->string.pointer + AUXMAC_START, AUXMAC_LEN);
> +	kfree(obj);
> +	return 0;
> +
> +auxmacinvalid:
> +	kfree(obj);
> +	memcpy(auxmac, "unavailable", 11);
> +	return 0;
> +}
> +
> +static struct ibm_struct auxmac_data = {
> +	.name = "auxmac",
> +};
> +
> +static ssize_t auxmac_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	if (strncmp(auxmac, "XXXXXXXXXXXX", AUXMAC_LEN) == 0)
> +		memcpy(auxmac, "disabled", 9);
> +
> +	return sysfs_emit(buf, "%s\n", auxmac);
> +}
> +static DEVICE_ATTR_RO(auxmac);
> +
> +static struct attribute *auxmac_attributes[] = {
> +	&dev_attr_auxmac.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group auxmac_attr_group = {
> +	.attrs = auxmac_attributes,
> +};
> +
>  /* --------------------------------------------------------------------- */
>  
>  static struct attribute *tpacpi_driver_attributes[] = {
> @@ -10843,6 +10915,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>  	&proxsensor_attr_group,
>  	&kbdlang_attr_group,
>  	&dprc_attr_group,
> +	&auxmac_attr_group,
>  	NULL,
>  };
>  
> @@ -11414,6 +11487,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = tpacpi_dprc_init,
>  		.data = &dprc_driver_data,
>  	},
> +	{
> +		.init = auxmac_init,
> +		.data = &auxmac_data,
> +	},
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)

Re: [PATCH] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Mark Pearson]

On Wed, Sep 13, 2023, at 11:58 AM, Hans de Goede wrote:
> Hi Fernando,
>
> On 9/6/23 21:52, Fernando Eckhardt Valle wrote:
>> Newer Thinkpads have a feature called Mac Address Passthrough.
>> This patch provides a sysfs interface that userspace can use
>> to get this auxiliary mac address.
>> 
>> Signed-off-by: Fernando Eckhardt Valle <fevalle@ipt.br>
>
> Thank you for your patch. 
>
> At a minimum for this patch to be accepted you will need
> to document the new sysfs interface in:
>
> Documentation/admin-guide/laptops/thinkpad-acpi.rst
>
> But I wonder if we should export this information to
> userspace in this way ?
>
> The reason why I'm wondering is because mac-address passthrough
> in case of using e.g. Lenovo Thunderbolt docks is already
> supported by the kernel by code for this in drivers/net/usb/r8152.c :
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/usb/r8152.c#n1613
>
> So I'm wondering if we really need this, is there a planned
> userspace API consumer of the new sysfs interface ?
>
> Or is this only intended as a way for a user to query this, iow
> is this purely intended for informational purposes ?
>
Hi Hans,

We've previously had strong pushback from the maintainers in the net tree that the MAC passthru should not be done there and should be done in user-space. I'd have to dig up the threads, but there was a preference for it to not be done in the kernel (and some frustrations at having vendor specific changes in the net driver).

We've also seen various timing issues (some related to ME FW doing it's thing) that makes it tricky to handle in the kernel - with added delays being needed leading to patches that can't be accepted.

This approach is one of the steps towards fixing this. Fernando did discuss and review this with me beforehand (apologies - I meant to add a note saying I'd been involved). If you think there is a better approach please let us know, but we figured as this is a Lenovo specific thing it made sense to have it here in thinkpad_acpi.

There will be a consumer (I think it's a script and udev rule) to update the MAC if a passthru-MAC address is provided via the BIOS. This will be open-source, but we haven't really figured out how to release it yet.

Fernando - please correct anything I've gotten wrong!

Mark

Re: [PATCH] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Hans de Goede]

Hi Mark, Fernando,

On 9/13/23 18:41, Mark Pearson wrote:
> 
> 
> On Wed, Sep 13, 2023, at 11:58 AM, Hans de Goede wrote:
>> Hi Fernando,
>>
>> On 9/6/23 21:52, Fernando Eckhardt Valle wrote:
>>> Newer Thinkpads have a feature called Mac Address Passthrough.
>>> This patch provides a sysfs interface that userspace can use
>>> to get this auxiliary mac address.
>>>
>>> Signed-off-by: Fernando Eckhardt Valle <fevalle@ipt.br>
>>
>> Thank you for your patch. 
>>
>> At a minimum for this patch to be accepted you will need
>> to document the new sysfs interface in:
>>
>> Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>
>> But I wonder if we should export this information to
>> userspace in this way ?
>>
>> The reason why I'm wondering is because mac-address passthrough
>> in case of using e.g. Lenovo Thunderbolt docks is already
>> supported by the kernel by code for this in drivers/net/usb/r8152.c :
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/usb/r8152.c#n1613
>>
>> So I'm wondering if we really need this, is there a planned
>> userspace API consumer of the new sysfs interface ?
>>
>> Or is this only intended as a way for a user to query this, iow
>> is this purely intended for informational purposes ?
>>
> Hi Hans,
> 
> We've previously had strong pushback from the maintainers in the net tree that the MAC passthru should not be done there and should be done in user-space. I'd have to dig up the threads, but there was a preference for it to not be done in the kernel (and some frustrations at having vendor specific changes in the net driver).
> 
> We've also seen various timing issues (some related to ME FW doing it's thing) that makes it tricky to handle in the kernel - with added delays being needed leading to patches that can't be accepted.
> 
> This approach is one of the steps towards fixing this. Fernando did discuss and review this with me beforehand (apologies - I meant to add a note saying I'd been involved). If you think there is a better approach please let us know, but we figured as this is a Lenovo specific thing it made sense to have it here in thinkpad_acpi.
> 
> There will be a consumer (I think it's a script and udev rule) to update the MAC if a passthru-MAC address is provided via the BIOS. This will be open-source, but we haven't really figured out how to release it yet.
> 
> Fernando - please correct anything I've gotten wrong!

Ah that is all good to know. That pretty much takes care of
my objections / answers my questions.

Fernando can you please submit a v2 which:

1. Adds documentation as mentioned already
2. Moves the special handling of "XXXXXXXXXXXX" from show()
   to init() (writing to auxmac[] in show() is a bit weird,
   also we only need to do this once, so it is init code)

Regards,

Hans

Re: [PATCH] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Fernando Eckhardt Valle (FIPT)]

Hi Mark, hi Hans,

All correct Mark, thanks!

Thanks for the review Hans, I'm already doing the v2.
Just a note: there are some Lenovo Thunderbolt 4 docks that have 2 ethernet devices inside, an intel ethernet device (loads igc driver, works with CPUs with vPRO feature) and a realtek ethernet device (realtek driver, works with CPU without vPRO feature). At this moment mac addr pass through doesn't work correctly in these docking station if the Thinkpad CPU has vPRO feature, and as Mark said, the idea is to release an open source application to work with the mac addr pt correctly.

Regards,
Fernando

________________________________________
From: Hans de Goede <hdegoede@redhat.com>
Sent: Wednesday, September 13, 2023 1:53 PM
To: Mark Pearson; Fernando Eckhardt Valle (FIPT); Henrique de Moraes Holschuh; markgross@kernel.org; ibm-acpi-devel@lists.sourceforge.net; platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: sysfs interface to auxmac

Hi Mark, Fernando,

On 9/13/23 18:41, Mark Pearson wrote:
>
>
> On Wed, Sep 13, 2023, at 11:58 AM, Hans de Goede wrote:
>> Hi Fernando,
>>
>> On 9/6/23 21:52, Fernando Eckhardt Valle wrote:
>>> Newer Thinkpads have a feature called Mac Address Passthrough.
>>> This patch provides a sysfs interface that userspace can use
>>> to get this auxiliary mac address.
>>>
>>> Signed-off-by: Fernando Eckhardt Valle <fevalle@ipt.br>
>>
>> Thank you for your patch.
>>
>> At a minimum for this patch to be accepted you will need
>> to document the new sysfs interface in:
>>
>> Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>
>> But I wonder if we should export this information to
>> userspace in this way ?
>>
>> The reason why I'm wondering is because mac-address passthrough
>> in case of using e.g. Lenovo Thunderbolt docks is already
>> supported by the kernel by code for this in drivers/net/usb/r8152.c :
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/usb/r8152.c#n1613
>>
>> So I'm wondering if we really need this, is there a planned
>> userspace API consumer of the new sysfs interface ?
>>
>> Or is this only intended as a way for a user to query this, iow
>> is this purely intended for informational purposes ?
>>
> Hi Hans,
>
> We've previously had strong pushback from the maintainers in the net tree that the MAC passthru should not be done there and should be done in user-space. I'd have to dig up the threads, but there was a preference for it to not be done in the kernel (and some frustrations at having vendor specific changes in the net driver).
>
> We've also seen various timing issues (some related to ME FW doing it's thing) that makes it tricky to handle in the kernel - with added delays being needed leading to patches that can't be accepted.
>
> This approach is one of the steps towards fixing this. Fernando did discuss and review this with me beforehand (apologies - I meant to add a note saying I'd been involved). If you think there is a better approach please let us know, but we figured as this is a Lenovo specific thing it made sense to have it here in thinkpad_acpi.
>
> There will be a consumer (I think it's a script and udev rule) to update the MAC if a passthru-MAC address is provided via the BIOS. This will be open-source, but we haven't really figured out how to release it yet.
>
> Fernando - please correct anything I've gotten wrong!

Ah that is all good to know. That pretty much takes care of
my objections / answers my questions.

Fernando can you please submit a v2 which:

1. Adds documentation as mentioned already
2. Moves the special handling of "XXXXXXXXXXXX" from show()
   to init() (writing to auxmac[] in show() is a bit weird,
   also we only need to do this once, so it is init code)

Regards,

Hans