| Message ID | 20230303144432.2108677-1-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ipu3-cio2: support multiple sensors and VCMs with HID name | expand |
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.
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 >
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.
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. >
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;