Re: [PATCH v2 1/6] platform/x86: serdev_helpers: Add get_serdev_controller_from_parent() helper

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2-Dec-24 5:34 PM, Ilpo Järvinen wrote:
> On Sat, 16 Nov 2024, Hans de Goede wrote:
> 
>> The x86-android-tablets code needs to be able to get a serdev_controller
>> device from a PCI parent, rather then by the ACPI HID+UID of the parent,
>> because on some tablets the UARTs are enumerated as PCI devices instead
>> of ACPI devices.
>>
>> Split the code to walk the device hierarchy to find the serdev_controller
>> from its parents out into a get_serdev_controller_from_parent() helper
>> so that the x86-android-tablets code can re-use it.
>>
>> Reviewed-by: Andy Shevchenko <andy@kernel.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/serdev_helpers.h | 60 +++++++++++++++------------
>>  1 file changed, 34 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/platform/x86/serdev_helpers.h b/drivers/platform/x86/serdev_helpers.h
>> index bcf3a0c356ea..b592b9ff6d93 100644
>> --- a/drivers/platform/x86/serdev_helpers.h
>> +++ b/drivers/platform/x86/serdev_helpers.h
>> @@ -22,32 +22,14 @@
>>  #include <linux/string.h>
>>  
>>  static inline struct device *
>> -get_serdev_controller(const char *serial_ctrl_hid,
>> -		      const char *serial_ctrl_uid,
>> -		      int serial_ctrl_port,
>> -		      const char *serdev_ctrl_name)
>> +get_serdev_controller_from_parent(struct device *ctrl_dev,
>> +				  int serial_ctrl_port,
>> +				  const char *serdev_ctrl_name)
>>  {
>> -	struct device *ctrl_dev, *child;
>> -	struct acpi_device *ctrl_adev;
>> +	struct device *child;
>>  	char name[32];
>>  	int i;
>>  
>> -	ctrl_adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
>> -	if (!ctrl_adev) {
>> -		pr_err("error could not get %s/%s serial-ctrl adev\n",
>> -		       serial_ctrl_hid, serial_ctrl_uid);
>> -		return ERR_PTR(-ENODEV);
>> -	}
>> -
>> -	/* get_first_physical_node() returns a weak ref */
>> -	ctrl_dev = get_device(acpi_get_first_physical_node(ctrl_adev));
>> -	if (!ctrl_dev) {
>> -		pr_err("error could not get %s/%s serial-ctrl physical node\n",
>> -		       serial_ctrl_hid, serial_ctrl_uid);
>> -		ctrl_dev = ERR_PTR(-ENODEV);
>> -		goto put_ctrl_adev;
>> -	}
>> -
>>  	/* Walk host -> uart-ctrl -> port -> serdev-ctrl */
>>  	for (i = 0; i < 3; i++) {
>>  		switch (i) {
>> @@ -67,14 +49,40 @@ get_serdev_controller(const char *serial_ctrl_hid,
>>  		put_device(ctrl_dev);
>>  		if (!child) {
>>  			pr_err("error could not find '%s' device\n", name);
>> -			ctrl_dev = ERR_PTR(-ENODEV);
>> -			goto put_ctrl_adev;
>> +			return ERR_PTR(-ENODEV);
>>  		}
>>  
>>  		ctrl_dev = child;
>>  	}
>>  
>> -put_ctrl_adev:
>> -	acpi_dev_put(ctrl_adev);
>>  	return ctrl_dev;
>>  }
>> +
>> +static inline struct device *
>> +get_serdev_controller(const char *serial_ctrl_hid,
>> +		      const char *serial_ctrl_uid,
>> +		      int serial_ctrl_port,
>> +		      const char *serdev_ctrl_name)
>> +{
>> +	struct acpi_device *adev;
>> +	struct device *parent;
>> +
>> +	adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1);
>> +	if (!adev) {
>> +		pr_err("error could not get %s/%s serial-ctrl adev\n",
>> +		       serial_ctrl_hid, serial_ctrl_uid);
> 
> Hi,
> 
> With the current code (and I suppose this moved too), W=1 build detects 
> that dell_uart_bl_pdev_probe() passed NULL which is then being formatted 
> here with %s. While it "works", it would be useful to solve the warning 
> and perhaps "/(null)" appearing in the print is also confusing to user 
> so maybe do another patch to change serial_ctrl_uid to e.g.:
> 
> 	serial_ctrl_uid ?: "*"
> 
> (There's another print below with the same problem).

Ack, for v3 I'll insert a new patch in the series before this patch
fixing this in the original code, including a Fixes: tag of the commit
introducing this.

Regards,

Hans




> 
> --
>  i.
> 
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	/* get_first_physical_node() returns a weak ref */
>> +	parent = get_device(acpi_get_first_physical_node(adev));
>> +	acpi_dev_put(adev);
>> +	if (!parent) {
>> +		pr_err("error could not get %s/%s serial-ctrl physical node\n",
>> +		       serial_ctrl_hid, serial_ctrl_uid);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	/* This puts our reference on parent and returns a ref on the ctrl */
>> +	return get_serdev_controller_from_parent(parent, serial_ctrl_port, serdev_ctrl_name);
>> +}
>>
>