[PATCH] media: ipu3-cio2: support multiple sensors and VCMs with HID name

[PATCH] media: ipu3-cio2: support multiple sensors and VCMs with HID name

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

In current cio2-bridge, it is using the hid as node name to register
software node and swnode will create kobject and sysfs entry with
the node name, if there are multiple sensors and VCMs which are sharing
same HID name, it will cause the software nodes registration failure:

[ 7.142311] sysfs: cannot create duplicate filename '/kernel/software_nodes/dw9714'
...
[ 7.142328] Call Trace:
[ 7.142330]  <TASK>
[ 7.142336]  dump_stack_lvl+0x49/0x63
[ 7.142341]  dump_stack+0x10/0x16
[ 7.142343]  sysfs_warn_dup.cold+0x17/0x2b
[ 7.142346]  sysfs_create_dir_ns+0xbc/0xd0
[ 7.142351]  kobject_add_internal+0xb1/0x2b0
[ 7.142356]  kobject_init_and_add+0x71/0xa0
[ 7.142360]  swnode_register+0x136/0x210
[ 7.142363]  software_node_register+0xd2/0x120
[ 7.142364]  software_node_register_nodes+0x83/0xf0
[ 7.142366]  ? acpi_get_physical_device_location+0x65/0xc0
[ 7.142371]  cio2_bridge_init+0x82a/0xb5e
...
[ 7.142448] kobject_add_internal failed for dw9714 with -EEXIST,
don't try to register things with the same name in the same directory.

One solution is appending the sensor link(Mipi Port) in SSDB as suffix
of the node name to fix this problem.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c | 14 ++++++++++----
 drivers/media/pci/intel/ipu3/cio2-bridge.h |  2 +-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index dfefe0d8aa95..1ce1acb18f3f 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -212,6 +212,7 @@ static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
 						  struct cio2_sensor *sensor)
 {
 	struct software_node *nodes = sensor->swnodes;
+	char vcm_name[ACPI_ID_LEN + 4];
 
 	cio2_bridge_init_swnode_names(sensor);
 
@@ -229,9 +230,12 @@ static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
 						sensor->node_names.endpoint,
 						&nodes[SWNODE_CIO2_PORT],
 						sensor->cio2_properties);
-	if (sensor->ssdb.vcmtype)
-		nodes[SWNODE_VCM] =
-			NODE_VCM(cio2_vcm_types[sensor->ssdb.vcmtype - 1]);
+	if (sensor->ssdb.vcmtype) {
+		scnprintf(vcm_name, sizeof(vcm_name), "%s-%u",
+			  cio2_vcm_types[sensor->ssdb.vcmtype - 1],
+			  sensor->ssdb.link);
+		nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
+	}
 
 	cio2_bridge_init_swnode_group(sensor);
 }
@@ -295,7 +299,6 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
 		}
 
 		sensor = &bridge->sensors[bridge->n_sensors];
-		strscpy(sensor->name, cfg->hid, sizeof(sensor->name));
 
 		ret = cio2_bridge_read_acpi_buffer(adev, "SSDB",
 						   &sensor->ssdb,
@@ -303,6 +306,9 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
 		if (ret)
 			goto err_put_adev;
 
+		scnprintf(sensor->name, sizeof(sensor->name), "%s-%u",
+			  cfg->hid, sensor->ssdb.link);
+
 		if (sensor->ssdb.vcmtype > ARRAY_SIZE(cio2_vcm_types)) {
 			dev_warn(&adev->dev, "Unknown VCM type %d\n",
 				 sensor->ssdb.vcmtype);
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
index b93b749c65bd..820ff518ef2b 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -113,7 +113,7 @@ struct cio2_sensor_config {
 };
 
 struct cio2_sensor {
-	char name[ACPI_ID_LEN];
+	char name[ACPI_ID_LEN + 4];
 	struct acpi_device *adev;
 	struct i2c_client *vcm_i2c_client;
 
-- 
2.39.1

Re: [PATCH] media: ipu3-cio2: support multiple sensors and VCMs with HID name

[Andy Shevchenko]

On Fri, Mar 03, 2023 at 10:44:32PM +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> In current cio2-bridge, it is using the hid as node name to register
> software node and swnode will create kobject and sysfs entry with
> the node name, if there are multiple sensors and VCMs which are sharing
> same HID name, it will cause the software nodes registration failure:
> 
> [ 7.142311] sysfs: cannot create duplicate filename '/kernel/software_nodes/dw9714'
> ...
> [ 7.142328] Call Trace:
> [ 7.142330]  <TASK>
> [ 7.142336]  dump_stack_lvl+0x49/0x63
> [ 7.142341]  dump_stack+0x10/0x16
> [ 7.142343]  sysfs_warn_dup.cold+0x17/0x2b
> [ 7.142346]  sysfs_create_dir_ns+0xbc/0xd0
> [ 7.142351]  kobject_add_internal+0xb1/0x2b0
> [ 7.142356]  kobject_init_and_add+0x71/0xa0
> [ 7.142360]  swnode_register+0x136/0x210
> [ 7.142363]  software_node_register+0xd2/0x120
> [ 7.142364]  software_node_register_nodes+0x83/0xf0
> [ 7.142366]  ? acpi_get_physical_device_location+0x65/0xc0
> [ 7.142371]  cio2_bridge_init+0x82a/0xb5e
> ...
> [ 7.142448] kobject_add_internal failed for dw9714 with -EEXIST,
> don't try to register things with the same name in the same directory.

Please cut unneeded context of the backtrace as it's explained in the
Submitting Patches documentation.

> One solution is appending the sensor link(Mipi Port) in SSDB as suffix
> of the node name to fix this problem.

...

> +	if (sensor->ssdb.vcmtype) {
> +		scnprintf(vcm_name, sizeof(vcm_name), "%s-%u",
> +			  cio2_vcm_types[sensor->ssdb.vcmtype - 1],
> +			  sensor->ssdb.link);

Is using 'c' variant a cargo cult? Otherwise explain, why dropping the last
part of the number is not a problem.

> +		nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
> +	}

...

> +		scnprintf(sensor->name, sizeof(sensor->name), "%s-%u",
> +			  cfg->hid, sensor->ssdb.link);


Ditto.

...

> -	char name[ACPI_ID_LEN];
> +	char name[ACPI_ID_LEN + 4];

Why 4 is chosen? This needs an explanation.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] media: ipu3-cio2: support multiple sensors and VCMs with HID name

[Cao, Bingbu]

Any, 

Thanks for your review.

------------------------------------------------------------------------
BRs,  
Intel VTG - Linux & Chrome IPU SW
Bingbu Cao 

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Monday, March 6, 2023 19:27
> To: Cao, Bingbu <bingbu.cao@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ailus@linux.intel.com;
> djrscally@gmail.com; bingbu.cao@linux.intel.com
> Subject: Re: [PATCH] media: ipu3-cio2: support multiple sensors and VCMs
> with HID name
> 
> On Fri, Mar 03, 2023 at 10:44:32PM +0800, bingbu.cao@intel.com wrote:
> > From: Bingbu Cao <bingbu.cao@intel.com>
> >
> > In current cio2-bridge, it is using the hid as node name to register
> > software node and swnode will create kobject and sysfs entry with the
> > node name, if there are multiple sensors and VCMs which are sharing
> > same HID name, it will cause the software nodes registration failure:
> >
> > [ 7.142311] sysfs: cannot create duplicate filename
> '/kernel/software_nodes/dw9714'
> > ...
> > [ 7.142328] Call Trace:
> > [ 7.142330]  <TASK>
> > [ 7.142336]  dump_stack_lvl+0x49/0x63
> > [ 7.142341]  dump_stack+0x10/0x16
> > [ 7.142343]  sysfs_warn_dup.cold+0x17/0x2b [ 7.142346]
> > sysfs_create_dir_ns+0xbc/0xd0 [ 7.142351]
> > kobject_add_internal+0xb1/0x2b0 [ 7.142356]
> > kobject_init_and_add+0x71/0xa0 [ 7.142360]
> > swnode_register+0x136/0x210 [ 7.142363]
> > software_node_register+0xd2/0x120 [ 7.142364]
> > software_node_register_nodes+0x83/0xf0
> > [ 7.142366]  ? acpi_get_physical_device_location+0x65/0xc0
> > [ 7.142371]  cio2_bridge_init+0x82a/0xb5e ...
> > [ 7.142448] kobject_add_internal failed for dw9714 with -EEXIST, don't
> > try to register things with the same name in the same directory.
> 
> Please cut unneeded context of the backtrace as it's explained in the
> Submitting Patches documentation.

Thanks, will amend in v2.

> 
> > One solution is appending the sensor link(Mipi Port) in SSDB as suffix
> > of the node name to fix this problem.
> 
> ...
> 
> > +	if (sensor->ssdb.vcmtype) {
> > +		scnprintf(vcm_name, sizeof(vcm_name), "%s-%u",
> > +			  cio2_vcm_types[sensor->ssdb.vcmtype - 1],
> > +			  sensor->ssdb.link);
> 
> Is using 'c' variant a cargo cult? Otherwise explain, why dropping the
> last part of the number is not a problem.

Sorry, I can't understand. What is cargo cult?

> 
> > +		nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
> > +	}
> 
> ...
> 
> > +		scnprintf(sensor->name, sizeof(sensor->name), "%s-%u",
> > +			  cfg->hid, sensor->ssdb.link);
> 
> 
> Ditto.
> 
> ...
> 
> > -	char name[ACPI_ID_LEN];
> > +	char name[ACPI_ID_LEN + 4];
> 
> Why 4 is chosen? This needs an explanation.

'link' is u8, so it is supposed to be max 4 characters along with '-'.


> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH] media: ipu3-cio2: support multiple sensors and VCMs with HID name

[Andy Shevchenko]

On Mon, Mar 06, 2023 at 01:33:30PM +0000, Cao, Bingbu wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Monday, March 6, 2023 19:27
> > On Fri, Mar 03, 2023 at 10:44:32PM +0800, bingbu.cao@intel.com wrote:

...

> > > +	if (sensor->ssdb.vcmtype) {
> > > +		scnprintf(vcm_name, sizeof(vcm_name), "%s-%u",
> > > +			  cio2_vcm_types[sensor->ssdb.vcmtype - 1],
> > > +			  sensor->ssdb.link);
> > 
> > Is using 'c' variant a cargo cult? Otherwise explain, why dropping the
> > last part of the number is not a problem.
> 
> Sorry, I can't understand. What is cargo cult?

Use of sCnprintf(). I.o.w. can you explain the point of using it instead of
simply snprintf()?

> > > +		nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
> > > +	}

...

> > > +		scnprintf(sensor->name, sizeof(sensor->name), "%s-%u",
> > > +			  cfg->hid, sensor->ssdb.link);

Ditto.

...

> > > -	char name[ACPI_ID_LEN];
> > > +	char name[ACPI_ID_LEN + 4];
> > 
> > Why 4 is chosen? This needs an explanation.
> 
> 'link' is u8, so it is supposed to be max 4 characters along with '-'.

It should be mentioned somewhere.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] media: ipu3-cio2: support multiple sensors and VCMs with HID name

[Bingbu Cao]

Andy,

On 3/7/23 12:35 AM, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 01:33:30PM +0000, Cao, Bingbu wrote:
>>> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Sent: Monday, March 6, 2023 19:27
>>> On Fri, Mar 03, 2023 at 10:44:32PM +0800, bingbu.cao@intel.com wrote:
> 
> ...
> 
>>>> +	if (sensor->ssdb.vcmtype) {
>>>> +		scnprintf(vcm_name, sizeof(vcm_name), "%s-%u",
>>>> +			  cio2_vcm_types[sensor->ssdb.vcmtype - 1],
>>>> +			  sensor->ssdb.link);
>>>
>>> Is using 'c' variant a cargo cult? Otherwise explain, why dropping the
>>> last part of the number is not a problem.
>>
>> Sorry, I can't understand. What is cargo cult?
> 
> Use of sCnprintf(). I.o.w. can you explain the point of using it instead of
> simply snprintf()?

Thanks, I see and will use simply snprintf() instead in v2.

> 
>>>> +		nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
>>>> +	}
> 
> ...
> 
>>>> +		scnprintf(sensor->name, sizeof(sensor->name), "%s-%u",
>>>> +			  cfg->hid, sensor->ssdb.link);
> 
> Ditto.
> 
> ...
> 
>>>> -	char name[ACPI_ID_LEN];
>>>> +	char name[ACPI_ID_LEN + 4];
>>>
>>> Why 4 is chosen? This needs an explanation.
>>
>> 'link' is u8, so it is supposed to be max 4 characters along with '-'.
> 
> It should be mentioned somewhere.

Will add a comment.

> 

-- 
Best regards,
Bingbu Cao