| Message ID | 20210115162559.20869-1-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: debugfs: use controller id instead of link_id | expand |
On 15-01-21, 16:25, Srinivas Kandagatla wrote: > link_id can be zero and if we have multiple controller instances > in a system like Qualcomm debugfs will end-up with duplicate namespace > resulting in incorrect debugfs entries. > > Using id should give a unique debugfs directory entry and should fix below > warning too. > "debugfs: Directory 'master-0' with parent 'soundwire' already present!" Yeah id is guaranteed to be unique so this will work. Applied, thanks
On 1/19/21 8:52 AM, Vinod Koul wrote: > On 15-01-21, 16:25, Srinivas Kandagatla wrote: >> link_id can be zero and if we have multiple controller instances >> in a system like Qualcomm debugfs will end-up with duplicate namespace >> resulting in incorrect debugfs entries. >> >> Using id should give a unique debugfs directory entry and should fix below >> warning too. >> "debugfs: Directory 'master-0' with parent 'soundwire' already present!" > > Yeah id is guaranteed to be unique so this will work. > > Applied, thanks This patch is a no-op for Intel, but I am not convinced it's the right fix or the definitions are not consistent. * @link_id: Link id number, can be 0 to N, unique for each Master * @id: bus system-wide unique id In the MIPI/DisCo definitions, a controller can have multiple masters. if you have multiple controllers, each of them should have their own debugfs directory IMHO. It's totally fine to have multiple bus/masters with a link_id == 0.
On 19/01/2021 15:54, Pierre-Louis Bossart wrote: > > > On 1/19/21 8:52 AM, Vinod Koul wrote: >> On 15-01-21, 16:25, Srinivas Kandagatla wrote: >>> link_id can be zero and if we have multiple controller instances >>> in a system like Qualcomm debugfs will end-up with duplicate namespace >>> resulting in incorrect debugfs entries. >>> >>> Using id should give a unique debugfs directory entry and should fix >>> below >>> warning too. >>> "debugfs: Directory 'master-0' with parent 'soundwire' already present!" >> >> Yeah id is guaranteed to be unique so this will work. >> >> Applied, thanks > > This patch is a no-op for Intel, but I am not convinced it's the right > fix or the definitions are not consistent. It depends if the intention is to represent full Hierarchy in debugfs, then I agree. Its was consistent even before! currently we have /sys/kernel/debug/soundwire/master-* Are you suggesting that we have something like this: /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID>/xyz-slave/master-<LINK-ID> ?? Or may be something much simpler like: /sys/kernel/debug/soundwire/master-<ID>.<LINK_ID> --srini > > * @link_id: Link id number, can be 0 to N, unique for each Master > * @id: bus system-wide unique id > > In the MIPI/DisCo definitions, a controller can have multiple masters. > if you have multiple controllers, each of them should have their own > debugfs directory IMHO. It's totally fine to have multiple bus/masters > with a link_id == 0.
>>>> link_id can be zero and if we have multiple controller instances >>>> in a system like Qualcomm debugfs will end-up with duplicate namespace >>>> resulting in incorrect debugfs entries. >>>> >>>> Using id should give a unique debugfs directory entry and should fix >>>> below >>>> warning too. >>>> "debugfs: Directory 'master-0' with parent 'soundwire' already >>>> present!" >>> >>> Yeah id is guaranteed to be unique so this will work. >>> >>> Applied, thanks >> >> This patch is a no-op for Intel, but I am not convinced it's the right >> fix or the definitions are not consistent. > > It depends if the intention is to represent full Hierarchy in debugfs, > then I agree. Its was consistent even before! Indeed, we don't currently have a notion of controller in debugfs. > currently we have > /sys/kernel/debug/soundwire/master-* > > Are you suggesting that we have something like this: > > /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? Yes this is what I was thinking about. > /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID>/xyz-slave/master-<LINK-ID> > ?? This would be for a bridge which to the best of my knowledge hasn't been implemented by anyone (clocking and command/control timing issues). > Or may be something much simpler like: > > /sys/kernel/debug/soundwire/master-<ID>.<LINK_ID> the bus->id is an IDA, which is useful for to avoid conflicts, but it's not really meaningful for debugfs. I remember seeing a case where we had links 2 and 4 enabled, and the bus->id were 0 and 1, a completely artificial value that doesn't really help in debugging.
On 19/01/2021 19:09, Pierre-Louis Bossart wrote: > >> currently we have >> /sys/kernel/debug/soundwire/master-* >> >> Are you suggesting that we have something like this: >> >> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? > > Yes this is what I was thinking about. Vinod/Pierre, One Question here, Why is link_id part of "struct sdw_bus", should it not be part of "struct sdw_master_device " ? Given that "There is one Link per each Master" --srini
On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: > > > On 19/01/2021 19:09, Pierre-Louis Bossart wrote: >> >>> currently we have >>> /sys/kernel/debug/soundwire/master-* >>> >>> Are you suggesting that we have something like this: >>> >>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? >> >> Yes this is what I was thinking about. > > Vinod/Pierre, > > One Question here, > > Why is link_id part of "struct sdw_bus", should it not be part of > "struct sdw_master_device " ? > > Given that "There is one Link per each Master" it's true that link == master == bus at the concept level. but we have an existing code base with different structures and we can't break too many things at once. In the existing flow, the 'bus' is created and setup first, the sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is already set. This routine only creates a device and in the rest of the code we keep using the 'bus' pointer, so there's no real short-term scope for moving the information into the 'sdw_master_device' structure - that would be a lot of surgery when nothing is really broken.
On 21/01/2021 15:12, Pierre-Louis Bossart wrote: > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: >> >> >> On 19/01/2021 19:09, Pierre-Louis Bossart wrote: >>> >>>> currently we have >>>> /sys/kernel/debug/soundwire/master-* >>>> >>>> Are you suggesting that we have something like this: >>>> >>>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? >>> >>> Yes this is what I was thinking about. >> >> Vinod/Pierre, >> >> One Question here, >> >> Why is link_id part of "struct sdw_bus", should it not be part of >> "struct sdw_master_device " ? >> >> Given that "There is one Link per each Master" > > it's true that link == master == bus at the concept level. > > but we have an existing code base with different structures and we can't > break too many things at once. > > In the existing flow, the 'bus' is created and setup first, the > sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is > already set. This routine only creates a device and in the rest of the > code we keep using the 'bus' pointer, so there's no real short-term > scope for moving the information into the 'sdw_master_device' structure > - that would be a lot of surgery when nothing is really broken. I totally agree! If I understand it correctly in Intel case there will be only one Link ID per bus. Does this change look good to you? ---------------->cut<--------------- diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c index b6cad0d59b7b..f22868614f09 100644 --- a/drivers/soundwire/debugfs.c +++ b/drivers/soundwire/debugfs.c @@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus) return; /* create the debugfs master-N */ + bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev), sdw_debugfs_root); snprintf(name, sizeof(name), "master-%d", bus->link_id); - bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root); + bus->debugfs = debugfs_create_dir(name, bus->controller_debugfs); } void sdw_bus_debugfs_exit(struct sdw_bus *bus) { - debugfs_remove_recursive(bus->debugfs); + debugfs_remove_recursive(bus->controller_debugfs); } #define RD_BUF (3 * PAGE_SIZE) diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index b198f471bea8..242bde30d8bd 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -877,6 +877,7 @@ struct sdw_bus { struct sdw_master_prop prop; struct list_head m_rt_list; #ifdef CONFIG_DEBUG_FS + struct dentry *controller_debugfs; struct dentry *debugfs; #endif struct sdw_defer defer_msg; ---------------->cut<--------------- With this change I get something like this on my board: ~# find /sys/kernel/debug/soundwire/ /sys/kernel/debug/soundwire/ /sys/kernel/debug/soundwire/sdw-master-2 /sys/kernel/debug/soundwire/sdw-master-2/master-0 /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4 /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4/registers /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3 /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3/registers /sys/kernel/debug/soundwire/sdw-master-1 /sys/kernel/debug/soundwire/sdw-master-1/master-0 /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3 /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3/registers /sys/kernel/debug/soundwire/sdw-master-0 /sys/kernel/debug/soundwire/sdw-master-0/master-0 /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4 /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4/registers Thanks, srini
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c > index b6cad0d59b7b..f22868614f09 100644 > --- a/drivers/soundwire/debugfs.c > +++ b/drivers/soundwire/debugfs.c > @@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus) > return; > > /* create the debugfs master-N */ > + bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev), > sdw_debugfs_root); bus->dev = &md->dev; dev_name(bus->dev) does not describe a controller at all but an individual master. We might as well just change the information as: snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id); you get the system unique ID and controller-relative ID, and you can decide to ignore one or the other on different platform.
On 21-01-21, 17:23, Srinivas Kandagatla wrote: > > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote: > > > > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: > > > > > > > > > On 19/01/2021 19:09, Pierre-Louis Bossart wrote: > > > > > > > > > currently we have > > > > > /sys/kernel/debug/soundwire/master-* > > > > > > > > > > Are you suggesting that we have something like this: > > > > > > > > > > /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? > > > > > > > > Yes this is what I was thinking about. > > > > > > Vinod/Pierre, > > > > > > One Question here, > > > > > > Why is link_id part of "struct sdw_bus", should it not be part of > > > "struct sdw_master_device " ? > > > > > > Given that "There is one Link per each Master" > > > > it's true that link == master == bus at the concept level. > > > > but we have an existing code base with different structures and we can't > > break too many things at once. > > > > In the existing flow, the 'bus' is created and setup first, the > > sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is > > already set. This routine only creates a device and in the rest of the > > code we keep using the 'bus' pointer, so there's no real short-term > > scope for moving the information into the 'sdw_master_device' structure > > - that would be a lot of surgery when nothing is really broken. > > I totally agree! > > If I understand it correctly in Intel case there will be only one Link ID > per bus. Yes IIUC there would be one link id per bus. the ida approach gives us unique id for each master,bus I would like to propose using that everywhere > > > Does this change look good to you? > > ---------------->cut<--------------- > > diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c > index b6cad0d59b7b..f22868614f09 100644 > --- a/drivers/soundwire/debugfs.c > +++ b/drivers/soundwire/debugfs.c > @@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus) > return; > > /* create the debugfs master-N */ > + bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev), > sdw_debugfs_root); > snprintf(name, sizeof(name), "master-%d", bus->link_id); > - bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root); > + bus->debugfs = debugfs_create_dir(name, bus->controller_debugfs); > } > > void sdw_bus_debugfs_exit(struct sdw_bus *bus) > { > - debugfs_remove_recursive(bus->debugfs); > + debugfs_remove_recursive(bus->controller_debugfs); > } > > #define RD_BUF (3 * PAGE_SIZE) > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index b198f471bea8..242bde30d8bd 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -877,6 +877,7 @@ struct sdw_bus { > struct sdw_master_prop prop; > struct list_head m_rt_list; > #ifdef CONFIG_DEBUG_FS > + struct dentry *controller_debugfs; > struct dentry *debugfs; > #endif > struct sdw_defer defer_msg; > > ---------------->cut<--------------- > > With this change I get something like this on my board: > > ~# find /sys/kernel/debug/soundwire/ > /sys/kernel/debug/soundwire/ > /sys/kernel/debug/soundwire/sdw-master-2 > /sys/kernel/debug/soundwire/sdw-master-2/master-0 > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4 > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4/registers > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3 > /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3/registers > /sys/kernel/debug/soundwire/sdw-master-1 > /sys/kernel/debug/soundwire/sdw-master-1/master-0 > /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3 > /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3/registers > /sys/kernel/debug/soundwire/sdw-master-0 > /sys/kernel/debug/soundwire/sdw-master-0/master-0 > /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4 > /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4/registers > > > > Thanks, > srini
On 2/1/21 4:14 AM, Vinod Koul wrote: > On 21-01-21, 17:23, Srinivas Kandagatla wrote: >> >> >> On 21/01/2021 15:12, Pierre-Louis Bossart wrote: >>> >>> >>> On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: >>>> >>>> >>>> On 19/01/2021 19:09, Pierre-Louis Bossart wrote: >>>>> >>>>>> currently we have >>>>>> /sys/kernel/debug/soundwire/master-* >>>>>> >>>>>> Are you suggesting that we have something like this: >>>>>> >>>>>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ?? >>>>> >>>>> Yes this is what I was thinking about. >>>> >>>> Vinod/Pierre, >>>> >>>> One Question here, >>>> >>>> Why is link_id part of "struct sdw_bus", should it not be part of >>>> "struct sdw_master_device " ? >>>> >>>> Given that "There is one Link per each Master" >>> >>> it's true that link == master == bus at the concept level. >>> >>> but we have an existing code base with different structures and we can't >>> break too many things at once. >>> >>> In the existing flow, the 'bus' is created and setup first, the >>> sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is >>> already set. This routine only creates a device and in the rest of the >>> code we keep using the 'bus' pointer, so there's no real short-term >>> scope for moving the information into the 'sdw_master_device' structure >>> - that would be a lot of surgery when nothing is really broken. >> >> I totally agree! >> >> If I understand it correctly in Intel case there will be only one Link ID >> per bus. > > Yes IIUC there would be one link id per bus. > > the ida approach gives us unique id for each master,bus I would like to > propose using that everywhere We have cases where link2 is not used but link0, 1 and 3 are. Using the IDA would result in master-0,1,2 being shown, that would throw the integrator off. the link_id is related to hardware and can tolerate gaps, the IDA is typically always increasing and is across the system, not controller specific. We can debate forever but both pieces of information are useful, so my recommendation is to use both: snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);
On 01-02-21, 10:10, Pierre-Louis Bossart wrote: > On 2/1/21 4:14 AM, Vinod Koul wrote: > > On 21-01-21, 17:23, Srinivas Kandagatla wrote: > > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote: > > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: > > > I totally agree! > > > > > > If I understand it correctly in Intel case there will be only one Link ID > > > per bus. > > > > Yes IIUC there would be one link id per bus. > > > > the ida approach gives us unique id for each master,bus I would like to > > propose using that everywhere > > We have cases where link2 is not used but link0, 1 and 3 are. > Using the IDA would result in master-0,1,2 being shown, that would throw the > integrator off. the link_id is related to hardware and can tolerate gaps, > the IDA is typically always increasing and is across the system, not > controller specific. > > We can debate forever but both pieces of information are useful, so my > recommendation is to use both: > > snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id); I agree we should use both, but does it really make sense for naming? We can keep name in ida and expose the link_id as a parameter for integrators to see in sysfs. Also, even in intel case you would run into issue if you have two independent controllers, am not sure if we ever get to see that, but I think link_id is unique for a controller and not across system, right? Thanks
On 2/1/21 10:18 PM, Vinod Koul wrote: > On 01-02-21, 10:10, Pierre-Louis Bossart wrote: >> On 2/1/21 4:14 AM, Vinod Koul wrote: >>> On 21-01-21, 17:23, Srinivas Kandagatla wrote: >>>> On 21/01/2021 15:12, Pierre-Louis Bossart wrote: >>>>> On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: > >>>> I totally agree! >>>> >>>> If I understand it correctly in Intel case there will be only one Link ID >>>> per bus. >>> >>> Yes IIUC there would be one link id per bus. >>> >>> the ida approach gives us unique id for each master,bus I would like to >>> propose using that everywhere >> >> We have cases where link2 is not used but link0, 1 and 3 are. >> Using the IDA would result in master-0,1,2 being shown, that would throw the >> integrator off. the link_id is related to hardware and can tolerate gaps, >> the IDA is typically always increasing and is across the system, not >> controller specific. >> >> We can debate forever but both pieces of information are useful, so my >> recommendation is to use both: >> >> snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id); > > I agree we should use both, but does it really make sense for naming? We > can keep name in ida and expose the link_id as a parameter for > integrators to see in sysfs. That would mean changing the meaning of sysfs properties: /* * The sysfs for properties reflects the MIPI description as given * in the MIPI DisCo spec * * Base file is: * sdw-master-N * |---- revision * |---- clk_stop_modes * |---- max_clk_freq * |---- clk_freq * |---- clk_gears * |---- default_row * |---- default_col * |---- dynamic_shape * |---- err_threshold */ N is the link ID in the spec. I am not convinced we'd do the community a service by unilaterally changing what an external spec means, or add a property that's kernel-defined while the rest is supposed to come from firmware. If you want to change the spec then you can contribute feedback in MIPI circles (MIPI have a mechanism for maintainers to provide such feedback without company/employer membership requirements) So either we add a sysfs layer that represents a controller (better in my opinion so that we can show the link/master count), or keep the existing hierarchy but expand the name with a unique ID so that Qualcomm don't get errors with duplicate sysfs link0 entries.
On 02-02-21, 10:43, Pierre-Louis Bossart wrote: > > > On 2/1/21 10:18 PM, Vinod Koul wrote: > > On 01-02-21, 10:10, Pierre-Louis Bossart wrote: > > > On 2/1/21 4:14 AM, Vinod Koul wrote: > > > > On 21-01-21, 17:23, Srinivas Kandagatla wrote: > > > > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote: > > > > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: > > > > > > > I totally agree! > > > > > > > > > > If I understand it correctly in Intel case there will be only one Link ID > > > > > per bus. > > > > > > > > Yes IIUC there would be one link id per bus. > > > > > > > > the ida approach gives us unique id for each master,bus I would like to > > > > propose using that everywhere > > > > > > We have cases where link2 is not used but link0, 1 and 3 are. > > > Using the IDA would result in master-0,1,2 being shown, that would throw the > > > integrator off. the link_id is related to hardware and can tolerate gaps, > > > the IDA is typically always increasing and is across the system, not > > > controller specific. > > > > > > We can debate forever but both pieces of information are useful, so my > > > recommendation is to use both: > > > > > > snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id); > > > > I agree we should use both, but does it really make sense for naming? We > > can keep name in ida and expose the link_id as a parameter for > > integrators to see in sysfs. > > That would mean changing the meaning of sysfs properties: > > /* > * The sysfs for properties reflects the MIPI description as given > * in the MIPI DisCo spec > * > * Base file is: > * sdw-master-N Key is "The sysfs for properties" is for property files. I am not sure how this implies for a number above. I was thinking of using ID for N here and add a link_id file below which represents the link-id property > * |---- revision > * |---- clk_stop_modes > * |---- max_clk_freq > * |---- clk_freq > * |---- clk_gears > * |---- default_row > * |---- default_col > * |---- dynamic_shape > * |---- err_threshold > */ > > N is the link ID in the spec. I am not convinced we'd do the community a > service by unilaterally changing what an external spec means, or add a > property that's kernel-defined while the rest is supposed to come from > firmware. If you want to change the spec then you can contribute feedback in > MIPI circles (MIPI have a mechanism for maintainers to provide such feedback > without company/employer membership requirements) > > So either we add a sysfs layer that represents a controller (better in my > opinion so that we can show the link/master count), or keep the existing > hierarchy but expand the name with a unique ID so that Qualcomm don't get > errors with duplicate sysfs link0 entries. Anyway we are late in cycle for this.. I am reverting this patch and we can arrive at consensus and fix this for next cycle Thanks
On 03-02-21, 16:44, Vinod Koul wrote: > On 02-02-21, 10:43, Pierre-Louis Bossart wrote: > > > > > > On 2/1/21 10:18 PM, Vinod Koul wrote: > > > On 01-02-21, 10:10, Pierre-Louis Bossart wrote: > > > > On 2/1/21 4:14 AM, Vinod Koul wrote: > > > > > On 21-01-21, 17:23, Srinivas Kandagatla wrote: > > > > > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote: > > > > > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote: > > > > > > > > > I totally agree! > > > > > > > > > > > > If I understand it correctly in Intel case there will be only one Link ID > > > > > > per bus. > > > > > > > > > > Yes IIUC there would be one link id per bus. > > > > > > > > > > the ida approach gives us unique id for each master,bus I would like to > > > > > propose using that everywhere > > > > > > > > We have cases where link2 is not used but link0, 1 and 3 are. > > > > Using the IDA would result in master-0,1,2 being shown, that would throw the > > > > integrator off. the link_id is related to hardware and can tolerate gaps, > > > > the IDA is typically always increasing and is across the system, not > > > > controller specific. > > > > > > > > We can debate forever but both pieces of information are useful, so my > > > > recommendation is to use both: > > > > > > > > snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id); > > > > > > I agree we should use both, but does it really make sense for naming? We > > > can keep name in ida and expose the link_id as a parameter for > > > integrators to see in sysfs. > > > > That would mean changing the meaning of sysfs properties: > > > > /* > > * The sysfs for properties reflects the MIPI description as given > > * in the MIPI DisCo spec > > * > > * Base file is: > > * sdw-master-N > > Key is "The sysfs for properties" is for property files. I am not sure > how this implies for a number above. I was thinking of using ID for N > here and add a link_id file below which represents the link-id property > > > * |---- revision > > * |---- clk_stop_modes > > * |---- max_clk_freq > > * |---- clk_freq > > * |---- clk_gears > > * |---- default_row > > * |---- default_col > > * |---- dynamic_shape > > * |---- err_threshold > > */ > > > > N is the link ID in the spec. I am not convinced we'd do the community a > > service by unilaterally changing what an external spec means, or add a > > property that's kernel-defined while the rest is supposed to come from > > firmware. If you want to change the spec then you can contribute feedback in > > MIPI circles (MIPI have a mechanism for maintainers to provide such feedback > > without company/employer membership requirements) > > > > So either we add a sysfs layer that represents a controller (better in my > > opinion so that we can show the link/master count), or keep the existing > > hierarchy but expand the name with a unique ID so that Qualcomm don't get > > errors with duplicate sysfs link0 entries. > > Anyway we are late in cycle for this.. I am reverting this patch and we > can arrive at consensus and fix this for next cycle Reverted and pushed out now
diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c index b6cad0d59b7b..5f9efa42bb25 100644 --- a/drivers/soundwire/debugfs.c +++ b/drivers/soundwire/debugfs.c @@ -19,7 +19,7 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus) return; /* create the debugfs master-N */ - snprintf(name, sizeof(name), "master-%d", bus->link_id); + snprintf(name, sizeof(name), "master-%d", bus->id); bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root); }
link_id can be zero and if we have multiple controller instances in a system like Qualcomm debugfs will end-up with duplicate namespace resulting in incorrect debugfs entries. Using id should give a unique debugfs directory entry and should fix below warning too. "debugfs: Directory 'master-0' with parent 'soundwire' already present!" Fixes: bf03473d5bcc ("soundwire: add debugfs support") Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)