> System with Type-C ports have a feature to expose an auxiliary
> persistent MAC address. This address is burned in at the
> factory.
>
> The intention of this address is to update the MAC address on
> Type-C docks containing an ethernet adapter to match the
> auxiliary address of the system connected to them.
>
> Signed-off-by: Mario Limonciello <mario_limoncie...@dell.com>
> ---
> drivers/platform/x86/dell-laptop.c | 61
> +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c
> b/drivers/platform/x86/dell-laptop.c
> index 2c2f02b..7d29690 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -87,6 +87,7 @@ static struct rfkill *wifi_rfkill;
> static struct rfkill *bluetooth_rfkill;
> static struct rfkill *wwan_rfkill;
> static bool force_rfkill;
> +static char *auxiliary_mac_address;
>
> module_param(force_rfkill, bool, 0444);
> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -273,6 +274,54 @@ static const struct dmi_system_id dell_quirks[]
> __initconst = {
> { }
> };
>
> +/* get_aux_mac
I'm not sure whether repeating the function's name in a comment placed
just above its definition is useful when not using kernel-doc.
CC'ing Matthew and Pali who are the maintainers of dell-laptop as this
boils down to their opinion (and you'll need their ack for the whole
patch anyway). Please CC them for any upcoming revisions of this patch
series.
> + * returns the auxiliary mac address
get_aux_mac() doesn't return the auxiliary MAC address, it stores it in
a variable and returns an error code. Please rephrase the comment to
avoid confusion.
> + * for assigning to a Type-C ethernet device
> + * such as that found in the Dell TB15 dock
> + */
> +static int get_aux_mac(void)
> +{
> + struct calling_interface_buffer *buffer;
> + unsigned char *extended_buffer;
> + size_t length;
> + int ret;
> +
> + buffer = dell_smbios_get_buffer();
> + length = 17;
> + extended_buffer = dell_smbios_send_extended_request(11, 6, &length);
> + ret = buffer->output[0];
> + if (ret != 0 || !extended_buffer) {
> + pr_debug("get_aux_mac: ext buffer: %p, len: %lu, ret: %d\n",
> + extended_buffer, length, ret);
> + auxiliary_mac_address = NULL;
This is redundant as auxiliary_mac_address is static and thus will be
initialized to NULL.
> + goto auxout;
I guess the label's name could be changed to "out" for consistency with
other functions in dell-laptop using only one goto label.
> + }
> +
> + /* address will be stored in byte 4-> */
This comment is now redundant.
> + auxiliary_mac_address = kmalloc(length, GFP_KERNEL);
> + memcpy(auxiliary_mac_address, extended_buffer, length);
> +
> + auxout:
> + dell_smbios_release_buffer();
> + return dell_smbios_error(ret);
> +}
> +
> +static ssize_t auxiliary_mac_show(struct device *dev,
> + struct device_attribute *attr, char *page)
Could you rename the variable "page" to "buf" for consistency with other
device attributes defined inside dell-laptop?
> +{
> + return sprintf(page, "%s\n", auxiliary_mac_address);
> +}
> +
> +static DEVICE_ATTR_RO(auxiliary_mac);
> +static struct attribute *dell_attributes[] = {
> + &dev_attr_auxiliary_mac.attr,
> + NULL
> +};
> +
> +static const struct attribute_group dell_attr_group = {
> + .attrs = dell_attributes,
> +};
> +
> /*
> * Derived from information in smbios-wireless-ctl:
> *
> @@ -392,7 +441,6 @@ static const struct dmi_system_id dell_quirks[]
> __initconst = {
> * cbArg1, byte0 = 0x13
> * cbRes1 Standard return codes (0, -1, -2)
> */
> -
It seems to me that removing this unrelated empty line is something
close to your heart ;)
> static int dell_rfkill_set(void *data, bool blocked)
> {
> struct calling_interface_buffer *buffer;
> @@ -2003,6 +2051,12 @@ static int __init dell_init(void)
> goto fail_rfkill;
> }
>
> + ret = get_aux_mac();
> + if (!ret) {
> + sysfs_create_group(&platform_device->dev.kobj,
> + &dell_attr_group);
> + }
The curly brackets are redundant here.
BTW, while this code will behave correctly when the requested length of
the extended buffer is 17, I cannot shake the premonition that bad
things will happen when someone copy-pastes your code for get_aux_mac()
while changing the requested length of the extended buffer to an invalid
value. In such a case dell_smbios_send_extended_request() would return
NULL, but the return value from the copy-pasted sibling of get_aux_mac()
would be 0, because dell_smbios_get_buffer() zeroes the SMI buffer and
dell_smbios_send_extended_request() would return so early that it
wouldn't even call dell_smbios_send_request(). Therefore, "if (!ret)"
would evaluate to true, even though the copy-pasted sibling of
get_aux_mac() would have encountered an error.
Perhaps I'm being overly paranoid, but maybe it won't hurt to do the
following instead:
get_aux_mac();
if (auxiliary_mac_address)
sysfs_create_group(&platform_device->dev.kobj,
&dell_attr_group);
and make get_aux_mac() return void. What do you think?
--
Best regards,
Michał Kępień
