Messages in this thread | | | Date | Wed, 29 Dec 2021 14:42:22 +0200 | Subject | Re: [PATCH v2 17/23] counter: intel-qep: Convert to new counter registration | From | Jarkko Nikula <> |
| |
On 12/28/21 12:56, Uwe Kleine-König wrote: > On Mon, Dec 27, 2021 at 05:02:39PM +0200, Jarkko Nikula wrote: >> On 12/27/21 11:45, Uwe Kleine-König wrote: >>> This fixes device lifetime issues where it was possible to free a live >>> struct device. >>> >>> Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral") >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> --- >>> drivers/counter/intel-qep.c | 28 ++++++++++++++++------------ >>> 1 file changed, 16 insertions(+), 12 deletions(-) >>> >> Should the Fixes tag rather be b6c50affda59 ("counter: Add character device >> interface") instead of when each drivers were introduced? I mean was it >> possible to hit the issue before /dev/counter was introduced? > > I'm not sure if there is an issue before this, but it was already wrong > before for sure. Maybe it's possible to hold a reference somehow via > sysfs? > > The thought that made me tag the individual driver commits was: With the > approach used to fix the issue all drivers need to be modified and an > unconverted driver doesn't benefit from the availability of > counter_alloc() / counter_add() if it isn't used. So all trees that > include b711f687a1c1 but not "counter: intel-qep: Convert to new counter > registration" are broken (more or less exploitable). So I think the > added Fixes line is the right choice. > Fair enough. Noticed only now after this patch (with patches 1, 4, 8 and 13 applied) second counter instance initialization fails:
[ 8.113999] sysfs: cannot create duplicate filename '/dev/char/244:0 [ 8.139604] Call Trace: [ 8.139606] <TASK> [ 8.139608] show_stack+0x3d/0x3f [ 8.139615] dump_stack_lvl+0x5b/0x82 [ 8.139619] dump_stack+0x10/0x12 [ 8.139621] sysfs_warn_dup.cold+0x17/0x27 [ 8.139624] sysfs_do_create_link_sd+0xc2/0xd0 [ 8.139629] sysfs_create_link+0x1c/0x30 [ 8.139631] device_add+0x54f/0x7c0 [ 8.139635] ? cdev_default_release+0x20/0x20 [ 8.139638] cdev_device_add+0x47/0x90 [ 8.139642] devm_counter_add+0x61/0xe0 [counter] [ 8.139647] intel_qep_probe+0x16f/0x1b0 ... [ 8.325081] intel-qep 0000:00:18.6: error -EEXIST: Failed to add counter [ 8.371793] intel-qep: probe of 0000:00:18.6 failed with error -17
Positive result is that following test doesn't produce an oops after a few iterations like before this patch.
while :; do { sleep 5; echo bang; } > /dev/counter0 & sleep 1; echo 0000:00:18.4 >/sys/bus/pci/drivers/intel-qep/unbind; sleep 8; echo 0000:00:18.4 >/sys/bus/pci/drivers/intel-qep/bind; done
| |