mbox series

[v9,00/33] Introduce the Counter character device interface

Message ID cover.1615293276.git.vilhelm.gray@gmail.com (mailing list archive)
Headers show
Series Introduce the Counter character device interface | expand

Message

William Breathitt Gray March 9, 2021, 1:19 p.m. UTC
Changes in v9:
 - Implemented example userspace counter application under tools/counter
 - Replaced extension*_name attributes with *_component_id attributes;
   this should hopefully be a more intuitive way to find the desired IDs
 - Changed to use regular spinlock because raw_spinlock is not needed
 - Implemented chrdev_lock mutex to limit chrdev to a single open() at a
   time
 - Improved struct counter_component documentation with examples
 - Reverted "counter_count_function" to "counter_function" naming change
   for drivers; individual maintainers can change this if they so desire
 - Utilized "return 0" in switch blocks to return early where possible
 - Utilized default cases in switch blocks to improve clarity and intent
 - Refactored counter_register to make use of cdev_add_device();
   counter_chrdev_add() has been simplified as a result
 - Inlined counter_chrdev_realloc_queue() because it is only used by the
   events_queue_size sysfs attribute
 - Replaced deprecated ida_simple_* calls with ida_alloc()/ida_free()
 - Made use of struct device "id" member to construct the cdev node name
 - Made use of kfifo_size() instead of rolling my own
 - Implemented changes necessary to migrate interrupt-cnt driver

Note that this revision is based on top of 5 prerequisite patches:
* counter: add IRQ or GPIO based counter
* dt-bindings: counter: add interrupt-counter binding
* counter: stm32-timer-cnt: fix ceiling miss-alignment with reload register
* counter: stm32-timer-cnt: fix ceiling write max value
* counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED

I pulled out a lot of bits and pieces to their own patches; hopefully
that makes reviewing this patchset much simpler than before. This
patchset is also available on my personal public git repo for anyone who
wants a quick way to clone:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v9

The patches preceding "counter: Internalize sysfs interface code" are
primarily cleanup and fixes that can be picked up and applied now to the
IIO tree if so desired. The "counter: Internalize sysfs interface code"
patch as well may be considered for pickup because it is relatively safe
and makes no changes to the userspace interface.

To summarize the main points of this patchset: there are no changes to
the existing Counter sysfs userspace interface; a Counter character
device interface is introduced that allows Counter events and associated
data to be read() by userspace; the events_configure() and
watch_validate() driver callbacks are introduced to support Counter
events; and IRQ support is added to the 104-QUAD-8 driver, serving as an
example of how to support the new Counter events functionality.

Something that should still be discussed: should the struct
counter_event "status" member be 8 bits or 32 bits wide? This member
will provide the return status (system error number) of an event
operation.

William Breathitt Gray (33):
  docs: counter: Consolidate Counter sysfs attributes documentation
  docs: counter: Fix spelling
  counter: 104-quad-8: Remove pointless comment
  counter: 104-quad-8: Return error when invalid mode during
    ceiling_write
  counter: 104-quad-8: Annotate hardware config module parameter
  counter: 104-quad-8: Add const qualifiers for
    quad8_preset_register_set
  counter: 104-quad-8: Add const qualifier for functions_list array
  counter: interrupt-cnt: Add const qualifier for functions_list array
  counter: microchip-tcb-capture: Add const qualifier for functions_list
    array
  counter: stm32-lptimer-cnt: Add const qualifier for functions_list
    array
  counter: stm32-timer-cnt: Add const qualifier for functions_list array
  counter: 104-quad-8: Add const qualifier for actions_list array
  counter: ftm-quaddec: Add const qualifier for actions_list array
  counter: interrupt-cnt: Add const qualifier for actions_list array
  counter: microchip-tcb-capture: Add const qualifier for actions_list
    array
  counter: stm32-lptimer-cnt: Add const qualifier for actions_list array
  counter: stm32-timer-cnt: Add const qualifier for actions_list array
  counter: Return error code on invalid modes
  counter: Standardize to ERANGE for limit exceeded errors
  counter: Rename counter_signal_value to counter_signal_level
  counter: Rename counter_count_function to counter_function
  counter: Internalize sysfs interface code
  counter: Update counter.h comments to reflect sysfs internalization
  docs: counter: Update to reflect sysfs internalization
  counter: Move counter enums to uapi header
  counter: Add character device interface
  docs: counter: Document character device interface
  tools/counter: Create Counter tools
  counter: Implement signalZ_action_component_id sysfs attribute
  counter: Implement *_component_id sysfs attributes
  counter: Implement events_queue_size sysfs attribute
  counter: 104-quad-8: Replace mutex with spinlock
  counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8

 Documentation/ABI/testing/sysfs-bus-counter   |  112 +-
 .../ABI/testing/sysfs-bus-counter-104-quad-8  |   61 -
 .../ABI/testing/sysfs-bus-counter-ftm-quaddec |   16 -
 Documentation/driver-api/generic-counter.rst  |  368 +++-
 .../userspace-api/ioctl/ioctl-number.rst      |    1 +
 MAINTAINERS                                   |    7 +-
 drivers/counter/104-quad-8.c                  |  739 ++++----
 drivers/counter/Kconfig                       |    6 +-
 drivers/counter/Makefile                      |    1 +
 drivers/counter/counter-chrdev.c              |  486 ++++++
 drivers/counter/counter-chrdev.h              |   14 +
 drivers/counter/counter-core.c                |  192 +++
 drivers/counter/counter-sysfs.c               |  953 +++++++++++
 drivers/counter/counter-sysfs.h               |   13 +
 drivers/counter/counter.c                     | 1496 -----------------
 drivers/counter/ftm-quaddec.c                 |   61 +-
 drivers/counter/interrupt-cnt.c               |   75 +-
 drivers/counter/microchip-tcb-capture.c       |  105 +-
 drivers/counter/stm32-lptimer-cnt.c           |  176 +-
 drivers/counter/stm32-timer-cnt.c             |  149 +-
 drivers/counter/ti-eqep.c                     |  221 +--
 include/linux/counter.h                       |  716 ++++----
 include/linux/counter_enum.h                  |   45 -
 include/uapi/linux/counter.h                  |  133 ++
 tools/Makefile                                |   13 +-
 tools/counter/Build                           |    1 +
 tools/counter/Makefile                        |   53 +
 tools/counter/counter_example.c               |   95 ++
 28 files changed, 3522 insertions(+), 2786 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
 delete mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ftm-quaddec
 create mode 100644 drivers/counter/counter-chrdev.c
 create mode 100644 drivers/counter/counter-chrdev.h
 create mode 100644 drivers/counter/counter-core.c
 create mode 100644 drivers/counter/counter-sysfs.c
 create mode 100644 drivers/counter/counter-sysfs.h
 delete mode 100644 drivers/counter/counter.c
 delete mode 100644 include/linux/counter_enum.h
 create mode 100644 include/uapi/linux/counter.h
 create mode 100644 tools/counter/Build
 create mode 100644 tools/counter/Makefile
 create mode 100644 tools/counter/counter_example.c


base-commit: 4ef57c4862e38e6034978d8b247a511292d7055a
prerequisite-patch-id: 41fda3a386861edad110c644567fad373a5a175e
prerequisite-patch-id: c6c2ab3173f5a0136d1e9b7b96ccd115fa35d66e
prerequisite-patch-id: 7e3cd78924d79890b690f3029e0d4f5b3902a73c
prerequisite-patch-id: 98f0a6c1d188a7dec01a5587fb7566ac637385a1
prerequisite-patch-id: 884299e23b6426ea43282e9701996e794cb6aa34

Comments

William Breathitt Gray March 14, 2021, 7:56 a.m. UTC | #1
On Fri, Mar 12, 2021 at 04:02:42PM +0100, Fabrice Gasnier wrote:
> On 3/9/21 2:19 PM, William Breathitt Gray wrote:
> > +static ssize_t enums_available_show(const u32 *const enums,
> > +				    const size_t num_enums,
> > +				    const char *const strs[], char *buf)
> > +{
> > +	size_t len = 0;
> > +	size_t index;
> > +
> > +	for (index = 0; index < num_enums; index++)
> > +		len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t strs_available_show(const struct counter_available *const avail,
> > +				   char *buf)
> > +{
> > +	size_t len = 0;
> > +	size_t index;
> > +
> > +	for (index = 0; index < avail->num_items; index++)
> > +		len += sysfs_emit(buf + len, "%s\n", avail->strs[index]);
> > +
> > +	return len;
> > +}
> 
> Hi William,
> 
> I was willing to do some testing on this series, on the stm32 counter
> drivers, since we released few fixes around them.
> 
> I tried to apply this series against current testing branch, with few
> patches applied (so it applies cleanly):
> - dt-bindings: counter: add interrupt-counter binding
> - counter: add IRQ or GPIO based counter
> - counter: stm32-timer-cnt: fix ceiling miss-alignment with reload register
> - counter: stm32-timer-cnt: fix ceiling write max value
>  counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
> 
> 
> For both the "stm32-lptimer-cnt" and "stm32-timer-cnt" drivers, I get a
> warning message and stack dump in "sysfs_emit" when reading the
> available functions from sysfs.
> I started to do some testing on v8 of this series last week. I didn't
> noticed that.
> 
> For both the "stm32-lptimer-cnt", there are 2 functions currently I get
> 1 stack dump. Only the "increase" function is printed correctly.
> 
> For the "stm32-timer-cnt", there are 4 functions currently, I get 3
> stack dumps. Only the "increase" function is printed correctly
> 
> Sample log for "stm32-timer-cnt:
> 
> root@stm32mp1:/sys/devices/platform/soc/44000000.timer/44000000.timer:counter/counter0#
> cat count0/function_available
> [ 4689.195506] ------------[ cut here ]------------
> [ 4689.198747] WARNING: CPU: 1 PID: 5841 at fs/sysfs/file.c:737
> sysfs_emit+0x88/0x94
> [ 4689.206233] invalid sysfs_emit: buf:f4a66208
> [ 4689.210553] Modules linked in: sha256_generic libsha256 sha256_arm
> cfg80211 panel_orisetech_otm8009a snd_soc_hdmi_codec
> snd_soc_stm32_sai_sub stm32_lptimers
> [ 4689.261444] CPU: 1 PID: 5841 Comm: cat Tainted: G        W
> 5.12.0-rc1 #534
> [ 4689.268999] Hardware name: STM32 (Device Tree Support)
> [ 4689.274166] [<c0310b38>] (unwind_backtrace) from [<c030b4ec>]
> (show_stack+0x10/0x14)
> [ 4689.281942] [<c030b4ec>] (show_stack) from [<c0fede70>]
> (dump_stack+0xc0/0xd4)
> [ 4689.289199] [<c0fede70>] (dump_stack) from [<c0345624>]
> (__warn+0xec/0x148)
> [ 4689.296194] [<c0345624>] (__warn) from [<c0fe9e90>]
> (warn_slowpath_fmt+0x98/0xbc)
> [ 4689.303714] [<c0fe9e90>] (warn_slowpath_fmt) from [<c0548ee0>]
> (sysfs_emit+0x88/0x94)
> [ 4689.311586] [<c0548ee0>] (sysfs_emit) from [<bf115de8>]
> (counter_comp_available_show+0x11c/0x1a4 [counter])
> [ 4689.321382] [<bf115de8>] (counter_comp_available_show [counter]) from
> [<c0a21b70>] (dev_attr_show+0x18/0x48)
> [ 4689.331263] [<c0a21b70>] (dev_attr_show) from [<c0549014>]
> (sysfs_kf_seq_show+0x88/0xf0)
> [ 4689.339394] [<c0549014>] (sysfs_kf_seq_show) from [<c04da6e8>]
> (seq_read_iter+0x1a4/0x554)
> [ 4689.347703] [<c04da6e8>] (seq_read_iter) from [<c04af6f0>]
> (vfs_read+0x1ac/0x2c4)
> [ 4689.355224] [<c04af6f0>] (vfs_read) from [<c04afc20>]
> (ksys_read+0x64/0xdc)
> [ 4689.362219] [<c04afc20>] (ksys_read) from [<c03000c0>]
> (ret_fast_syscall+0x0/0x58)
> [ 4689.369827] Exception stack(0xc7261fa8 to 0xc7261ff0)
> [ 4689.374906] 1fa0:                   00000000 00020000 00000003
> b6f35000 00020000 00000000
> [ 4689.383126] 1fc0: 00000000 00020000 b6f56ce0 00000003 00000003
> 00000000 00020000 00000000
> [ 4689.391344] 1fe0: 00000003 be8239a8 410bff27 4104c066
> ...
> 2 more stack dumps follow
> ...
> [ 4689.810479] ---[ end trace 59ed79949efe984c ]---
> increase
> 
> I get similar backtrace with other _available attributes:
> $ cat signal0_action_available
> $ cat signal1_action_available
> 
> Do you think I'm doing something wrong ?
> 
> I tested then "quadrature x4" on the timer driver... It seems all fine.
> 
> Best regards
> Fabrice
> 
> > +
> > +static ssize_t counter_comp_available_show(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   char *buf)
> > +{
> > +	const struct counter_attribute *const a = to_counter_attribute(attr);
> > +	const struct counter_count *const count = a->parent;
> > +	const struct counter_synapse *const synapse = a->comp.priv;
> > +	const struct counter_available *const avail = a->comp.priv;
> > +
> > +	switch (a->comp.type) {
> > +	case COUNTER_COMP_FUNCTION:
> > +		return enums_available_show(count->functions_list,
> > +					    count->num_functions,
> > +					    counter_function_str, buf);
> > +	case COUNTER_COMP_SYNAPSE_ACTION:
> > +		return enums_available_show(synapse->actions_list,
> > +					    synapse->num_actions,
> > +					    counter_synapse_action_str, buf);
> > +	case COUNTER_COMP_ENUM:
> > +		return strs_available_show(avail, buf);
> > +	case COUNTER_COMP_COUNT_MODE:
> > +		return enums_available_show(avail->enums, avail->num_items,
> > +					    counter_count_mode_str, buf);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}

Hi Fabrice,

I can confirm that I'm hitting this regression as well with the
104-quad-8 driver. The warning seems to be caused by the
offset_in_page(buf) check in sysfs_emit(). It looks like the first loop
in enums_available_show() calls sysfs_emit() correctly, but subsequent
loops have an invalid buf offset.

The enums_available_show() callback is rather simple: call sysfs_emit()
for each enum string and increment buf by the length written each time.
I haven't modified this function since v8, so I am somewhat confused
about why the buf offset would be invalid here now. I wonder if there
has been a change somewhere else in the kernel that is causing
sysfs_emit() to now return an incorrect length.

William Breathitt Gray
William Breathitt Gray March 14, 2021, 9:08 a.m. UTC | #2
On Sun, Mar 14, 2021 at 04:56:44PM +0900, William Breathitt Gray wrote:
> On Fri, Mar 12, 2021 at 04:02:42PM +0100, Fabrice Gasnier wrote:
> > On 3/9/21 2:19 PM, William Breathitt Gray wrote:
> > > +static ssize_t enums_available_show(const u32 *const enums,
> > > +				    const size_t num_enums,
> > > +				    const char *const strs[], char *buf)
> > > +{
> > > +	size_t len = 0;
> > > +	size_t index;
> > > +
> > > +	for (index = 0; index < num_enums; index++)
> > > +		len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +static ssize_t strs_available_show(const struct counter_available *const avail,
> > > +				   char *buf)
> > > +{
> > > +	size_t len = 0;
> > > +	size_t index;
> > > +
> > > +	for (index = 0; index < avail->num_items; index++)
> > > +		len += sysfs_emit(buf + len, "%s\n", avail->strs[index]);
> > > +
> > > +	return len;
> > > +}
> > 
> > Hi William,
> > 
> > I was willing to do some testing on this series, on the stm32 counter
> > drivers, since we released few fixes around them.
> > 
> > I tried to apply this series against current testing branch, with few
> > patches applied (so it applies cleanly):
> > - dt-bindings: counter: add interrupt-counter binding
> > - counter: add IRQ or GPIO based counter
> > - counter: stm32-timer-cnt: fix ceiling miss-alignment with reload register
> > - counter: stm32-timer-cnt: fix ceiling write max value
> >  counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
> > 
> > 
> > For both the "stm32-lptimer-cnt" and "stm32-timer-cnt" drivers, I get a
> > warning message and stack dump in "sysfs_emit" when reading the
> > available functions from sysfs.
> > I started to do some testing on v8 of this series last week. I didn't
> > noticed that.
> > 
> > For both the "stm32-lptimer-cnt", there are 2 functions currently I get
> > 1 stack dump. Only the "increase" function is printed correctly.
> > 
> > For the "stm32-timer-cnt", there are 4 functions currently, I get 3
> > stack dumps. Only the "increase" function is printed correctly
> > 
> > Sample log for "stm32-timer-cnt:
> > 
> > root@stm32mp1:/sys/devices/platform/soc/44000000.timer/44000000.timer:counter/counter0#
> > cat count0/function_available
> > [ 4689.195506] ------------[ cut here ]------------
> > [ 4689.198747] WARNING: CPU: 1 PID: 5841 at fs/sysfs/file.c:737
> > sysfs_emit+0x88/0x94
> > [ 4689.206233] invalid sysfs_emit: buf:f4a66208
> > [ 4689.210553] Modules linked in: sha256_generic libsha256 sha256_arm
> > cfg80211 panel_orisetech_otm8009a snd_soc_hdmi_codec
> > snd_soc_stm32_sai_sub stm32_lptimers
> > [ 4689.261444] CPU: 1 PID: 5841 Comm: cat Tainted: G        W
> > 5.12.0-rc1 #534
> > [ 4689.268999] Hardware name: STM32 (Device Tree Support)
> > [ 4689.274166] [<c0310b38>] (unwind_backtrace) from [<c030b4ec>]
> > (show_stack+0x10/0x14)
> > [ 4689.281942] [<c030b4ec>] (show_stack) from [<c0fede70>]
> > (dump_stack+0xc0/0xd4)
> > [ 4689.289199] [<c0fede70>] (dump_stack) from [<c0345624>]
> > (__warn+0xec/0x148)
> > [ 4689.296194] [<c0345624>] (__warn) from [<c0fe9e90>]
> > (warn_slowpath_fmt+0x98/0xbc)
> > [ 4689.303714] [<c0fe9e90>] (warn_slowpath_fmt) from [<c0548ee0>]
> > (sysfs_emit+0x88/0x94)
> > [ 4689.311586] [<c0548ee0>] (sysfs_emit) from [<bf115de8>]
> > (counter_comp_available_show+0x11c/0x1a4 [counter])
> > [ 4689.321382] [<bf115de8>] (counter_comp_available_show [counter]) from
> > [<c0a21b70>] (dev_attr_show+0x18/0x48)
> > [ 4689.331263] [<c0a21b70>] (dev_attr_show) from [<c0549014>]
> > (sysfs_kf_seq_show+0x88/0xf0)
> > [ 4689.339394] [<c0549014>] (sysfs_kf_seq_show) from [<c04da6e8>]
> > (seq_read_iter+0x1a4/0x554)
> > [ 4689.347703] [<c04da6e8>] (seq_read_iter) from [<c04af6f0>]
> > (vfs_read+0x1ac/0x2c4)
> > [ 4689.355224] [<c04af6f0>] (vfs_read) from [<c04afc20>]
> > (ksys_read+0x64/0xdc)
> > [ 4689.362219] [<c04afc20>] (ksys_read) from [<c03000c0>]
> > (ret_fast_syscall+0x0/0x58)
> > [ 4689.369827] Exception stack(0xc7261fa8 to 0xc7261ff0)
> > [ 4689.374906] 1fa0:                   00000000 00020000 00000003
> > b6f35000 00020000 00000000
> > [ 4689.383126] 1fc0: 00000000 00020000 b6f56ce0 00000003 00000003
> > 00000000 00020000 00000000
> > [ 4689.391344] 1fe0: 00000003 be8239a8 410bff27 4104c066
> > ...
> > 2 more stack dumps follow
> > ...
> > [ 4689.810479] ---[ end trace 59ed79949efe984c ]---
> > increase
> > 
> > I get similar backtrace with other _available attributes:
> > $ cat signal0_action_available
> > $ cat signal1_action_available
> > 
> > Do you think I'm doing something wrong ?
> > 
> > I tested then "quadrature x4" on the timer driver... It seems all fine.
> > 
> > Best regards
> > Fabrice
> > 
> > > +
> > > +static ssize_t counter_comp_available_show(struct device *dev,
> > > +					   struct device_attribute *attr,
> > > +					   char *buf)
> > > +{
> > > +	const struct counter_attribute *const a = to_counter_attribute(attr);
> > > +	const struct counter_count *const count = a->parent;
> > > +	const struct counter_synapse *const synapse = a->comp.priv;
> > > +	const struct counter_available *const avail = a->comp.priv;
> > > +
> > > +	switch (a->comp.type) {
> > > +	case COUNTER_COMP_FUNCTION:
> > > +		return enums_available_show(count->functions_list,
> > > +					    count->num_functions,
> > > +					    counter_function_str, buf);
> > > +	case COUNTER_COMP_SYNAPSE_ACTION:
> > > +		return enums_available_show(synapse->actions_list,
> > > +					    synapse->num_actions,
> > > +					    counter_synapse_action_str, buf);
> > > +	case COUNTER_COMP_ENUM:
> > > +		return strs_available_show(avail, buf);
> > > +	case COUNTER_COMP_COUNT_MODE:
> > > +		return enums_available_show(avail->enums, avail->num_items,
> > > +					    counter_count_mode_str, buf);
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> 
> Hi Fabrice,
> 
> I can confirm that I'm hitting this regression as well with the
> 104-quad-8 driver. The warning seems to be caused by the
> offset_in_page(buf) check in sysfs_emit(). It looks like the first loop
> in enums_available_show() calls sysfs_emit() correctly, but subsequent
> loops have an invalid buf offset.
> 
> The enums_available_show() callback is rather simple: call sysfs_emit()
> for each enum string and increment buf by the length written each time.
> I haven't modified this function since v8, so I am somewhat confused
> about why the buf offset would be invalid here now. I wonder if there
> has been a change somewhere else in the kernel that is causing
> sysfs_emit() to now return an incorrect length.
> 
> William Breathitt Gray

Fabrice,

Would you be able to check the values of buf and len before they enter
sysfs_emit()? I think redefining the enums_available_show() function
like this should suffice:

static ssize_t enums_available_show(const u32 *const enums,
                                    const size_t num_enums,
                                    const char *const strs[], char *buf)
{
        size_t len = 0;
        size_t index;

        for (index = 0; index < num_enums; index++){
                pr_info("buf: %p\tbuf+len: %p\tlen: %zu\n", buf, buf + len, len);
                len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
        }

        return len;
}

I want to see whether the issue is due to the sysfs_emit() return value
or the value of buf.

Thank you,

William Breathitt Gray
Fabrice Gasnier March 18, 2021, 9:21 a.m. UTC | #3
On 3/14/21 10:08 AM, William Breathitt Gray wrote:
> On Sun, Mar 14, 2021 at 04:56:44PM +0900, William Breathitt Gray wrote:
>> On Fri, Mar 12, 2021 at 04:02:42PM +0100, Fabrice Gasnier wrote:
>>> On 3/9/21 2:19 PM, William Breathitt Gray wrote:
>>>> +static ssize_t enums_available_show(const u32 *const enums,
>>>> +				    const size_t num_enums,
>>>> +				    const char *const strs[], char *buf)
>>>> +{
>>>> +	size_t len = 0;
>>>> +	size_t index;
>>>> +
>>>> +	for (index = 0; index < num_enums; index++)
>>>> +		len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
>>>> +
>>>> +	return len;
>>>> +}
>>>> +
>>>> +static ssize_t strs_available_show(const struct counter_available *const avail,
>>>> +				   char *buf)
>>>> +{
>>>> +	size_t len = 0;
>>>> +	size_t index;
>>>> +
>>>> +	for (index = 0; index < avail->num_items; index++)
>>>> +		len += sysfs_emit(buf + len, "%s\n", avail->strs[index]);
>>>> +
>>>> +	return len;
>>>> +}
>>>
>>> Hi William,
>>>
>>> I was willing to do some testing on this series, on the stm32 counter
>>> drivers, since we released few fixes around them.
>>>
>>> I tried to apply this series against current testing branch, with few
>>> patches applied (so it applies cleanly):
>>> - dt-bindings: counter: add interrupt-counter binding
>>> - counter: add IRQ or GPIO based counter
>>> - counter: stm32-timer-cnt: fix ceiling miss-alignment with reload register
>>> - counter: stm32-timer-cnt: fix ceiling write max value
>>>  counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
>>>
>>>
>>> For both the "stm32-lptimer-cnt" and "stm32-timer-cnt" drivers, I get a
>>> warning message and stack dump in "sysfs_emit" when reading the
>>> available functions from sysfs.
>>> I started to do some testing on v8 of this series last week. I didn't
>>> noticed that.
>>>
>>> For both the "stm32-lptimer-cnt", there are 2 functions currently I get
>>> 1 stack dump. Only the "increase" function is printed correctly.
>>>
>>> For the "stm32-timer-cnt", there are 4 functions currently, I get 3
>>> stack dumps. Only the "increase" function is printed correctly
>>>
>>> Sample log for "stm32-timer-cnt:
>>>
>>> root@stm32mp1:/sys/devices/platform/soc/44000000.timer/44000000.timer:counter/counter0#
>>> cat count0/function_available
>>> [ 4689.195506] ------------[ cut here ]------------
>>> [ 4689.198747] WARNING: CPU: 1 PID: 5841 at fs/sysfs/file.c:737
>>> sysfs_emit+0x88/0x94
>>> [ 4689.206233] invalid sysfs_emit: buf:f4a66208
>>> [ 4689.210553] Modules linked in: sha256_generic libsha256 sha256_arm
>>> cfg80211 panel_orisetech_otm8009a snd_soc_hdmi_codec
>>> snd_soc_stm32_sai_sub stm32_lptimers
>>> [ 4689.261444] CPU: 1 PID: 5841 Comm: cat Tainted: G        W
>>> 5.12.0-rc1 #534
>>> [ 4689.268999] Hardware name: STM32 (Device Tree Support)
>>> [ 4689.274166] [<c0310b38>] (unwind_backtrace) from [<c030b4ec>]
>>> (show_stack+0x10/0x14)
>>> [ 4689.281942] [<c030b4ec>] (show_stack) from [<c0fede70>]
>>> (dump_stack+0xc0/0xd4)
>>> [ 4689.289199] [<c0fede70>] (dump_stack) from [<c0345624>]
>>> (__warn+0xec/0x148)
>>> [ 4689.296194] [<c0345624>] (__warn) from [<c0fe9e90>]
>>> (warn_slowpath_fmt+0x98/0xbc)
>>> [ 4689.303714] [<c0fe9e90>] (warn_slowpath_fmt) from [<c0548ee0>]
>>> (sysfs_emit+0x88/0x94)
>>> [ 4689.311586] [<c0548ee0>] (sysfs_emit) from [<bf115de8>]
>>> (counter_comp_available_show+0x11c/0x1a4 [counter])
>>> [ 4689.321382] [<bf115de8>] (counter_comp_available_show [counter]) from
>>> [<c0a21b70>] (dev_attr_show+0x18/0x48)
>>> [ 4689.331263] [<c0a21b70>] (dev_attr_show) from [<c0549014>]
>>> (sysfs_kf_seq_show+0x88/0xf0)
>>> [ 4689.339394] [<c0549014>] (sysfs_kf_seq_show) from [<c04da6e8>]
>>> (seq_read_iter+0x1a4/0x554)
>>> [ 4689.347703] [<c04da6e8>] (seq_read_iter) from [<c04af6f0>]
>>> (vfs_read+0x1ac/0x2c4)
>>> [ 4689.355224] [<c04af6f0>] (vfs_read) from [<c04afc20>]
>>> (ksys_read+0x64/0xdc)
>>> [ 4689.362219] [<c04afc20>] (ksys_read) from [<c03000c0>]
>>> (ret_fast_syscall+0x0/0x58)
>>> [ 4689.369827] Exception stack(0xc7261fa8 to 0xc7261ff0)
>>> [ 4689.374906] 1fa0:                   00000000 00020000 00000003
>>> b6f35000 00020000 00000000
>>> [ 4689.383126] 1fc0: 00000000 00020000 b6f56ce0 00000003 00000003
>>> 00000000 00020000 00000000
>>> [ 4689.391344] 1fe0: 00000003 be8239a8 410bff27 4104c066
>>> ...
>>> 2 more stack dumps follow
>>> ...
>>> [ 4689.810479] ---[ end trace 59ed79949efe984c ]---
>>> increase
>>>
>>> I get similar backtrace with other _available attributes:
>>> $ cat signal0_action_available
>>> $ cat signal1_action_available
>>>
>>> Do you think I'm doing something wrong ?
>>>
>>> I tested then "quadrature x4" on the timer driver... It seems all fine.
>>>
>>> Best regards
>>> Fabrice
>>>
>>>> +
>>>> +static ssize_t counter_comp_available_show(struct device *dev,
>>>> +					   struct device_attribute *attr,
>>>> +					   char *buf)
>>>> +{
>>>> +	const struct counter_attribute *const a = to_counter_attribute(attr);
>>>> +	const struct counter_count *const count = a->parent;
>>>> +	const struct counter_synapse *const synapse = a->comp.priv;
>>>> +	const struct counter_available *const avail = a->comp.priv;
>>>> +
>>>> +	switch (a->comp.type) {
>>>> +	case COUNTER_COMP_FUNCTION:
>>>> +		return enums_available_show(count->functions_list,
>>>> +					    count->num_functions,
>>>> +					    counter_function_str, buf);
>>>> +	case COUNTER_COMP_SYNAPSE_ACTION:
>>>> +		return enums_available_show(synapse->actions_list,
>>>> +					    synapse->num_actions,
>>>> +					    counter_synapse_action_str, buf);
>>>> +	case COUNTER_COMP_ENUM:
>>>> +		return strs_available_show(avail, buf);
>>>> +	case COUNTER_COMP_COUNT_MODE:
>>>> +		return enums_available_show(avail->enums, avail->num_items,
>>>> +					    counter_count_mode_str, buf);
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +}
>>
>> Hi Fabrice,
>>
>> I can confirm that I'm hitting this regression as well with the
>> 104-quad-8 driver. The warning seems to be caused by the
>> offset_in_page(buf) check in sysfs_emit(). It looks like the first loop
>> in enums_available_show() calls sysfs_emit() correctly, but subsequent
>> loops have an invalid buf offset.
>>
>> The enums_available_show() callback is rather simple: call sysfs_emit()
>> for each enum string and increment buf by the length written each time.
>> I haven't modified this function since v8, so I am somewhat confused
>> about why the buf offset would be invalid here now. I wonder if there
>> has been a change somewhere else in the kernel that is causing
>> sysfs_emit() to now return an incorrect length.
>>
>> William Breathitt Gray
> 
> Fabrice,
> 
> Would you be able to check the values of buf and len before they enter
> sysfs_emit()? I think redefining the enums_available_show() function
> like this should suffice:
> 
> static ssize_t enums_available_show(const u32 *const enums,
>                                     const size_t num_enums,
>                                     const char *const strs[], char *buf)
> {
>         size_t len = 0;
>         size_t index;
> 
>         for (index = 0; index < num_enums; index++){
>                 pr_info("buf: %p\tbuf+len: %p\tlen: %zu\n", buf, buf + len, len);
>                 len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
>         }
> 
>         return len;
> }
> 
> I want to see whether the issue is due to the sysfs_emit() return value
> or the value of buf.

Hi William,

Sorry for the delay,

I'm getting strange results on buf+len. Here's the result I'm getting
with same test as above:

[  170.190995] buf: 5daf3333    buf+len: 5daf3333       len: 0
[  170.194383] buf: 5daf3333    buf+len: 22c37039       len: 9
[  170.199268] ------------[ cut here ]------------
...
[  170.404810] buf: 5daf3333    buf+len: 22c37039       len: 9
[  170.409663] ------------[ cut here ]------------
...
[  170.615265] buf: 5daf3333    buf+len: 22c37039       len: 9
[  170.620117] ------------[ cut here ]------------
...
increase

Hope this helps,
Fabrice

> 
> Thank you,
> 
> William Breathitt Gray
>
Fabrice Gasnier March 18, 2021, 10:10 a.m. UTC | #4
On 3/18/21 10:21 AM, Fabrice Gasnier wrote:
> On 3/14/21 10:08 AM, William Breathitt Gray wrote:
>> On Sun, Mar 14, 2021 at 04:56:44PM +0900, William Breathitt Gray wrote:
>>> On Fri, Mar 12, 2021 at 04:02:42PM +0100, Fabrice Gasnier wrote:
>>>> On 3/9/21 2:19 PM, William Breathitt Gray wrote:
>>>>> +static ssize_t enums_available_show(const u32 *const enums,
>>>>> +				    const size_t num_enums,
>>>>> +				    const char *const strs[], char *buf)
>>>>> +{
>>>>> +	size_t len = 0;
>>>>> +	size_t index;
>>>>> +
>>>>> +	for (index = 0; index < num_enums; index++)
>>>>> +		len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
>>>>> +
>>>>> +	return len;
>>>>> +}
>>>>> +
>>>>> +static ssize_t strs_available_show(const struct counter_available *const avail,
>>>>> +				   char *buf)
>>>>> +{
>>>>> +	size_t len = 0;
>>>>> +	size_t index;
>>>>> +
>>>>> +	for (index = 0; index < avail->num_items; index++)
>>>>> +		len += sysfs_emit(buf + len, "%s\n", avail->strs[index]);
>>>>> +
>>>>> +	return len;
>>>>> +}
>>>>
>>>> Hi William,
>>>>
>>>> I was willing to do some testing on this series, on the stm32 counter
>>>> drivers, since we released few fixes around them.
>>>>
>>>> I tried to apply this series against current testing branch, with few
>>>> patches applied (so it applies cleanly):
>>>> - dt-bindings: counter: add interrupt-counter binding
>>>> - counter: add IRQ or GPIO based counter
>>>> - counter: stm32-timer-cnt: fix ceiling miss-alignment with reload register
>>>> - counter: stm32-timer-cnt: fix ceiling write max value
>>>>  counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
>>>>
>>>>
>>>> For both the "stm32-lptimer-cnt" and "stm32-timer-cnt" drivers, I get a
>>>> warning message and stack dump in "sysfs_emit" when reading the
>>>> available functions from sysfs.
>>>> I started to do some testing on v8 of this series last week. I didn't
>>>> noticed that.
>>>>
>>>> For both the "stm32-lptimer-cnt", there are 2 functions currently I get
>>>> 1 stack dump. Only the "increase" function is printed correctly.
>>>>
>>>> For the "stm32-timer-cnt", there are 4 functions currently, I get 3
>>>> stack dumps. Only the "increase" function is printed correctly
>>>>
>>>> Sample log for "stm32-timer-cnt:
>>>>
>>>> root@stm32mp1:/sys/devices/platform/soc/44000000.timer/44000000.timer:counter/counter0#
>>>> cat count0/function_available
>>>> [ 4689.195506] ------------[ cut here ]------------
>>>> [ 4689.198747] WARNING: CPU: 1 PID: 5841 at fs/sysfs/file.c:737
>>>> sysfs_emit+0x88/0x94
>>>> [ 4689.206233] invalid sysfs_emit: buf:f4a66208
>>>> [ 4689.210553] Modules linked in: sha256_generic libsha256 sha256_arm
>>>> cfg80211 panel_orisetech_otm8009a snd_soc_hdmi_codec
>>>> snd_soc_stm32_sai_sub stm32_lptimers
>>>> [ 4689.261444] CPU: 1 PID: 5841 Comm: cat Tainted: G        W
>>>> 5.12.0-rc1 #534
>>>> [ 4689.268999] Hardware name: STM32 (Device Tree Support)
>>>> [ 4689.274166] [<c0310b38>] (unwind_backtrace) from [<c030b4ec>]
>>>> (show_stack+0x10/0x14)
>>>> [ 4689.281942] [<c030b4ec>] (show_stack) from [<c0fede70>]
>>>> (dump_stack+0xc0/0xd4)
>>>> [ 4689.289199] [<c0fede70>] (dump_stack) from [<c0345624>]
>>>> (__warn+0xec/0x148)
>>>> [ 4689.296194] [<c0345624>] (__warn) from [<c0fe9e90>]
>>>> (warn_slowpath_fmt+0x98/0xbc)
>>>> [ 4689.303714] [<c0fe9e90>] (warn_slowpath_fmt) from [<c0548ee0>]
>>>> (sysfs_emit+0x88/0x94)
>>>> [ 4689.311586] [<c0548ee0>] (sysfs_emit) from [<bf115de8>]
>>>> (counter_comp_available_show+0x11c/0x1a4 [counter])
>>>> [ 4689.321382] [<bf115de8>] (counter_comp_available_show [counter]) from
>>>> [<c0a21b70>] (dev_attr_show+0x18/0x48)
>>>> [ 4689.331263] [<c0a21b70>] (dev_attr_show) from [<c0549014>]
>>>> (sysfs_kf_seq_show+0x88/0xf0)
>>>> [ 4689.339394] [<c0549014>] (sysfs_kf_seq_show) from [<c04da6e8>]
>>>> (seq_read_iter+0x1a4/0x554)
>>>> [ 4689.347703] [<c04da6e8>] (seq_read_iter) from [<c04af6f0>]
>>>> (vfs_read+0x1ac/0x2c4)
>>>> [ 4689.355224] [<c04af6f0>] (vfs_read) from [<c04afc20>]
>>>> (ksys_read+0x64/0xdc)
>>>> [ 4689.362219] [<c04afc20>] (ksys_read) from [<c03000c0>]
>>>> (ret_fast_syscall+0x0/0x58)
>>>> [ 4689.369827] Exception stack(0xc7261fa8 to 0xc7261ff0)
>>>> [ 4689.374906] 1fa0:                   00000000 00020000 00000003
>>>> b6f35000 00020000 00000000
>>>> [ 4689.383126] 1fc0: 00000000 00020000 b6f56ce0 00000003 00000003
>>>> 00000000 00020000 00000000
>>>> [ 4689.391344] 1fe0: 00000003 be8239a8 410bff27 4104c066
>>>> ...
>>>> 2 more stack dumps follow
>>>> ...
>>>> [ 4689.810479] ---[ end trace 59ed79949efe984c ]---
>>>> increase
>>>>
>>>> I get similar backtrace with other _available attributes:
>>>> $ cat signal0_action_available
>>>> $ cat signal1_action_available
>>>>
>>>> Do you think I'm doing something wrong ?
>>>>
>>>> I tested then "quadrature x4" on the timer driver... It seems all fine.
>>>>
>>>> Best regards
>>>> Fabrice
>>>>
>>>>> +
>>>>> +static ssize_t counter_comp_available_show(struct device *dev,
>>>>> +					   struct device_attribute *attr,
>>>>> +					   char *buf)
>>>>> +{
>>>>> +	const struct counter_attribute *const a = to_counter_attribute(attr);
>>>>> +	const struct counter_count *const count = a->parent;
>>>>> +	const struct counter_synapse *const synapse = a->comp.priv;
>>>>> +	const struct counter_available *const avail = a->comp.priv;
>>>>> +
>>>>> +	switch (a->comp.type) {
>>>>> +	case COUNTER_COMP_FUNCTION:
>>>>> +		return enums_available_show(count->functions_list,
>>>>> +					    count->num_functions,
>>>>> +					    counter_function_str, buf);
>>>>> +	case COUNTER_COMP_SYNAPSE_ACTION:
>>>>> +		return enums_available_show(synapse->actions_list,
>>>>> +					    synapse->num_actions,
>>>>> +					    counter_synapse_action_str, buf);
>>>>> +	case COUNTER_COMP_ENUM:
>>>>> +		return strs_available_show(avail, buf);
>>>>> +	case COUNTER_COMP_COUNT_MODE:
>>>>> +		return enums_available_show(avail->enums, avail->num_items,
>>>>> +					    counter_count_mode_str, buf);
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +}
>>>
>>> Hi Fabrice,
>>>
>>> I can confirm that I'm hitting this regression as well with the
>>> 104-quad-8 driver. The warning seems to be caused by the
>>> offset_in_page(buf) check in sysfs_emit(). It looks like the first loop
>>> in enums_available_show() calls sysfs_emit() correctly, but subsequent
>>> loops have an invalid buf offset.
>>>
>>> The enums_available_show() callback is rather simple: call sysfs_emit()
>>> for each enum string and increment buf by the length written each time.
>>> I haven't modified this function since v8, so I am somewhat confused
>>> about why the buf offset would be invalid here now. I wonder if there
>>> has been a change somewhere else in the kernel that is causing
>>> sysfs_emit() to now return an incorrect length.
>>>
>>> William Breathitt Gray
>>
>> Fabrice,
>>
>> Would you be able to check the values of buf and len before they enter
>> sysfs_emit()? I think redefining the enums_available_show() function
>> like this should suffice:
>>
>> static ssize_t enums_available_show(const u32 *const enums,
>>                                     const size_t num_enums,
>>                                     const char *const strs[], char *buf)
>> {
>>         size_t len = 0;
>>         size_t index;
>>
>>         for (index = 0; index < num_enums; index++){
>>                 pr_info("buf: %p\tbuf+len: %p\tlen: %zu\n", buf, buf + len, len);
>>                 len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
>>         }
>>
>>         return len;
>> }
>>
>> I want to see whether the issue is due to the sysfs_emit() return value
>> or the value of buf.
> 
> Hi William,
> 
> Sorry for the delay,
> 
> I'm getting strange results on buf+len. Here's the result I'm getting
> with same test as above:
> 
> [  170.190995] buf: 5daf3333    buf+len: 5daf3333       len: 0
> [  170.194383] buf: 5daf3333    buf+len: 22c37039       len: 9
> [  170.199268] ------------[ cut here ]------------
> ...
> [  170.404810] buf: 5daf3333    buf+len: 22c37039       len: 9
> [  170.409663] ------------[ cut here ]------------
> ...
> [  170.615265] buf: 5daf3333    buf+len: 22c37039       len: 9
> [  170.620117] ------------[ cut here ]------------
> ...
> increase

William,

I did the same, with %px instead of %p, and i'm getting:

[  124.001041] buf: c60fb000    buf+len: c60fb000       len: 0
[  124.009442] buf: c60fb000    buf+len: c60fb009       len: 9
[  124.019118] ------------[ cut here ]------------
...
So, I believe this is caused by the offset_in_page(buf) check, in
sysfs_emit().

I also double checked it on the v8 patchset, and I already had the same
behavior. So I likely didn't checked the available attrs earlier. Sorry
for this confusion.

Best Regards,
Fabrice

> 
> Hope this helps,
> Fabrice
> 
>>
>> Thank you,
>>
>> William Breathitt Gray
>>
William Breathitt Gray March 18, 2021, 2:22 p.m. UTC | #5
On Thu, Mar 18, 2021 at 11:10:29AM +0100, Fabrice Gasnier wrote:
> On 3/18/21 10:21 AM, Fabrice Gasnier wrote:
> > On 3/14/21 10:08 AM, William Breathitt Gray wrote:
> >> On Sun, Mar 14, 2021 at 04:56:44PM +0900, William Breathitt Gray wrote:
> >>> On Fri, Mar 12, 2021 at 04:02:42PM +0100, Fabrice Gasnier wrote:
> >>>> On 3/9/21 2:19 PM, William Breathitt Gray wrote:
> >>>>> +static ssize_t enums_available_show(const u32 *const enums,
> >>>>> +				    const size_t num_enums,
> >>>>> +				    const char *const strs[], char *buf)
> >>>>> +{
> >>>>> +	size_t len = 0;
> >>>>> +	size_t index;
> >>>>> +
> >>>>> +	for (index = 0; index < num_enums; index++)
> >>>>> +		len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
> >>>>> +
> >>>>> +	return len;
> >>>>> +}
> >>>>> +
> >>>>> +static ssize_t strs_available_show(const struct counter_available *const avail,
> >>>>> +				   char *buf)
> >>>>> +{
> >>>>> +	size_t len = 0;
> >>>>> +	size_t index;
> >>>>> +
> >>>>> +	for (index = 0; index < avail->num_items; index++)
> >>>>> +		len += sysfs_emit(buf + len, "%s\n", avail->strs[index]);
> >>>>> +
> >>>>> +	return len;
> >>>>> +}
> >>>>
> >>>> Hi William,
> >>>>
> >>>> I was willing to do some testing on this series, on the stm32 counter
> >>>> drivers, since we released few fixes around them.
> >>>>
> >>>> I tried to apply this series against current testing branch, with few
> >>>> patches applied (so it applies cleanly):
> >>>> - dt-bindings: counter: add interrupt-counter binding
> >>>> - counter: add IRQ or GPIO based counter
> >>>> - counter: stm32-timer-cnt: fix ceiling miss-alignment with reload register
> >>>> - counter: stm32-timer-cnt: fix ceiling write max value
> >>>>  counter: stm32-timer-cnt: Report count function when SLAVE_MODE_DISABLED
> >>>>
> >>>>
> >>>> For both the "stm32-lptimer-cnt" and "stm32-timer-cnt" drivers, I get a
> >>>> warning message and stack dump in "sysfs_emit" when reading the
> >>>> available functions from sysfs.
> >>>> I started to do some testing on v8 of this series last week. I didn't
> >>>> noticed that.
> >>>>
> >>>> For both the "stm32-lptimer-cnt", there are 2 functions currently I get
> >>>> 1 stack dump. Only the "increase" function is printed correctly.
> >>>>
> >>>> For the "stm32-timer-cnt", there are 4 functions currently, I get 3
> >>>> stack dumps. Only the "increase" function is printed correctly
> >>>>
> >>>> Sample log for "stm32-timer-cnt:
> >>>>
> >>>> root@stm32mp1:/sys/devices/platform/soc/44000000.timer/44000000.timer:counter/counter0#
> >>>> cat count0/function_available
> >>>> [ 4689.195506] ------------[ cut here ]------------
> >>>> [ 4689.198747] WARNING: CPU: 1 PID: 5841 at fs/sysfs/file.c:737
> >>>> sysfs_emit+0x88/0x94
> >>>> [ 4689.206233] invalid sysfs_emit: buf:f4a66208
> >>>> [ 4689.210553] Modules linked in: sha256_generic libsha256 sha256_arm
> >>>> cfg80211 panel_orisetech_otm8009a snd_soc_hdmi_codec
> >>>> snd_soc_stm32_sai_sub stm32_lptimers
> >>>> [ 4689.261444] CPU: 1 PID: 5841 Comm: cat Tainted: G        W
> >>>> 5.12.0-rc1 #534
> >>>> [ 4689.268999] Hardware name: STM32 (Device Tree Support)
> >>>> [ 4689.274166] [<c0310b38>] (unwind_backtrace) from [<c030b4ec>]
> >>>> (show_stack+0x10/0x14)
> >>>> [ 4689.281942] [<c030b4ec>] (show_stack) from [<c0fede70>]
> >>>> (dump_stack+0xc0/0xd4)
> >>>> [ 4689.289199] [<c0fede70>] (dump_stack) from [<c0345624>]
> >>>> (__warn+0xec/0x148)
> >>>> [ 4689.296194] [<c0345624>] (__warn) from [<c0fe9e90>]
> >>>> (warn_slowpath_fmt+0x98/0xbc)
> >>>> [ 4689.303714] [<c0fe9e90>] (warn_slowpath_fmt) from [<c0548ee0>]
> >>>> (sysfs_emit+0x88/0x94)
> >>>> [ 4689.311586] [<c0548ee0>] (sysfs_emit) from [<bf115de8>]
> >>>> (counter_comp_available_show+0x11c/0x1a4 [counter])
> >>>> [ 4689.321382] [<bf115de8>] (counter_comp_available_show [counter]) from
> >>>> [<c0a21b70>] (dev_attr_show+0x18/0x48)
> >>>> [ 4689.331263] [<c0a21b70>] (dev_attr_show) from [<c0549014>]
> >>>> (sysfs_kf_seq_show+0x88/0xf0)
> >>>> [ 4689.339394] [<c0549014>] (sysfs_kf_seq_show) from [<c04da6e8>]
> >>>> (seq_read_iter+0x1a4/0x554)
> >>>> [ 4689.347703] [<c04da6e8>] (seq_read_iter) from [<c04af6f0>]
> >>>> (vfs_read+0x1ac/0x2c4)
> >>>> [ 4689.355224] [<c04af6f0>] (vfs_read) from [<c04afc20>]
> >>>> (ksys_read+0x64/0xdc)
> >>>> [ 4689.362219] [<c04afc20>] (ksys_read) from [<c03000c0>]
> >>>> (ret_fast_syscall+0x0/0x58)
> >>>> [ 4689.369827] Exception stack(0xc7261fa8 to 0xc7261ff0)
> >>>> [ 4689.374906] 1fa0:                   00000000 00020000 00000003
> >>>> b6f35000 00020000 00000000
> >>>> [ 4689.383126] 1fc0: 00000000 00020000 b6f56ce0 00000003 00000003
> >>>> 00000000 00020000 00000000
> >>>> [ 4689.391344] 1fe0: 00000003 be8239a8 410bff27 4104c066
> >>>> ...
> >>>> 2 more stack dumps follow
> >>>> ...
> >>>> [ 4689.810479] ---[ end trace 59ed79949efe984c ]---
> >>>> increase
> >>>>
> >>>> I get similar backtrace with other _available attributes:
> >>>> $ cat signal0_action_available
> >>>> $ cat signal1_action_available
> >>>>
> >>>> Do you think I'm doing something wrong ?
> >>>>
> >>>> I tested then "quadrature x4" on the timer driver... It seems all fine.
> >>>>
> >>>> Best regards
> >>>> Fabrice
> >>>>
> >>>>> +
> >>>>> +static ssize_t counter_comp_available_show(struct device *dev,
> >>>>> +					   struct device_attribute *attr,
> >>>>> +					   char *buf)
> >>>>> +{
> >>>>> +	const struct counter_attribute *const a = to_counter_attribute(attr);
> >>>>> +	const struct counter_count *const count = a->parent;
> >>>>> +	const struct counter_synapse *const synapse = a->comp.priv;
> >>>>> +	const struct counter_available *const avail = a->comp.priv;
> >>>>> +
> >>>>> +	switch (a->comp.type) {
> >>>>> +	case COUNTER_COMP_FUNCTION:
> >>>>> +		return enums_available_show(count->functions_list,
> >>>>> +					    count->num_functions,
> >>>>> +					    counter_function_str, buf);
> >>>>> +	case COUNTER_COMP_SYNAPSE_ACTION:
> >>>>> +		return enums_available_show(synapse->actions_list,
> >>>>> +					    synapse->num_actions,
> >>>>> +					    counter_synapse_action_str, buf);
> >>>>> +	case COUNTER_COMP_ENUM:
> >>>>> +		return strs_available_show(avail, buf);
> >>>>> +	case COUNTER_COMP_COUNT_MODE:
> >>>>> +		return enums_available_show(avail->enums, avail->num_items,
> >>>>> +					    counter_count_mode_str, buf);
> >>>>> +	default:
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +}
> >>>
> >>> Hi Fabrice,
> >>>
> >>> I can confirm that I'm hitting this regression as well with the
> >>> 104-quad-8 driver. The warning seems to be caused by the
> >>> offset_in_page(buf) check in sysfs_emit(). It looks like the first loop
> >>> in enums_available_show() calls sysfs_emit() correctly, but subsequent
> >>> loops have an invalid buf offset.
> >>>
> >>> The enums_available_show() callback is rather simple: call sysfs_emit()
> >>> for each enum string and increment buf by the length written each time.
> >>> I haven't modified this function since v8, so I am somewhat confused
> >>> about why the buf offset would be invalid here now. I wonder if there
> >>> has been a change somewhere else in the kernel that is causing
> >>> sysfs_emit() to now return an incorrect length.
> >>>
> >>> William Breathitt Gray
> >>
> >> Fabrice,
> >>
> >> Would you be able to check the values of buf and len before they enter
> >> sysfs_emit()? I think redefining the enums_available_show() function
> >> like this should suffice:
> >>
> >> static ssize_t enums_available_show(const u32 *const enums,
> >>                                     const size_t num_enums,
> >>                                     const char *const strs[], char *buf)
> >> {
> >>         size_t len = 0;
> >>         size_t index;
> >>
> >>         for (index = 0; index < num_enums; index++){
> >>                 pr_info("buf: %p\tbuf+len: %p\tlen: %zu\n", buf, buf + len, len);
> >>                 len += sysfs_emit(buf + len, "%s\n", strs[enums[index]]);
> >>         }
> >>
> >>         return len;
> >> }
> >>
> >> I want to see whether the issue is due to the sysfs_emit() return value
> >> or the value of buf.
> > 
> > Hi William,
> > 
> > Sorry for the delay,
> > 
> > I'm getting strange results on buf+len. Here's the result I'm getting
> > with same test as above:
> > 
> > [  170.190995] buf: 5daf3333    buf+len: 5daf3333       len: 0
> > [  170.194383] buf: 5daf3333    buf+len: 22c37039       len: 9
> > [  170.199268] ------------[ cut here ]------------
> > ...
> > [  170.404810] buf: 5daf3333    buf+len: 22c37039       len: 9
> > [  170.409663] ------------[ cut here ]------------
> > ...
> > [  170.615265] buf: 5daf3333    buf+len: 22c37039       len: 9
> > [  170.620117] ------------[ cut here ]------------
> > ...
> > increase
> 
> William,
> 
> I did the same, with %px instead of %p, and i'm getting:
> 
> [  124.001041] buf: c60fb000    buf+len: c60fb000       len: 0
> [  124.009442] buf: c60fb000    buf+len: c60fb009       len: 9
> [  124.019118] ------------[ cut here ]------------
> ...
> So, I believe this is caused by the offset_in_page(buf) check, in
> sysfs_emit().
> 
> I also double checked it on the v8 patchset, and I already had the same
> behavior. So I likely didn't checked the available attrs earlier. Sorry
> for this confusion.
> 
> Best Regards,
> Fabrice

Ah, I forgot %p doesn't show the true address. Okay so it looks like we
can't use sysfs_emit() with an offset. I'll change these sysfs_emit()
lines to use scnprintf() instead and that should prevent the warnings
from triggering.

Thanks,

William Breathitt Gray