From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
To: William Breathitt Gray <vilhelm.gray@gmail.com>
Cc: <jic23@kernel.org>, <kamel.bouhara@bootlin.com>,
<gwendal@chromium.org>, <david@lechnology.com>,
<linux-iio@vger.kernel.org>, <patrick.havelange@essensium.com>,
<alexandre.belloni@bootlin.com>, <mcoquelin.stm32@gmail.com>,
<linux-kernel@vger.kernel.org>, <o.rempel@pengutronix.de>,
Dan Carpenter <dan.carpenter@oracle.com>, <kernel@pengutronix.de>,
<fabrice.gasnier@foss.st.com>, <syednwaris@gmail.com>,
<linux-stm32@st-md-mailman.stormreply.com>,
<linux-arm-kernel@lists.infradead.org>,
<alexandre.torgue@foss.st.com>
Subject: Re: [Linux-stm32] [PATCH v9 22/33] counter: Internalize sysfs interface code
Date: Thu, 18 Mar 2021 11:10:29 +0100 [thread overview]
Message-ID: <3c3183c4-c9da-709b-dff3-ab5e407e042b@foss.st.com> (raw)
In-Reply-To: <fa974bd9-96bc-3647-a5b7-5d268e459753@foss.st.com>
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
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-18 10:13 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-09 13:19 [PATCH v9 00/33] Introduce the Counter character device interface William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 01/33] docs: counter: Consolidate Counter sysfs attributes documentation William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 02/33] docs: counter: Fix spelling William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 03/33] counter: 104-quad-8: Remove pointless comment William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 04/33] counter: 104-quad-8: Return error when invalid mode during ceiling_write William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 05/33] counter: 104-quad-8: Annotate hardware config module parameter William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 06/33] counter: 104-quad-8: Add const qualifiers for quad8_preset_register_set William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 07/33] counter: 104-quad-8: Add const qualifier for functions_list array William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 08/33] counter: interrupt-cnt: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 09/33] counter: microchip-tcb-capture: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 10/33] counter: stm32-lptimer-cnt: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 11/33] counter: stm32-timer-cnt: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 12/33] counter: 104-quad-8: Add const qualifier for actions_list array William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 13/33] counter: ftm-quaddec: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 14/33] counter: interrupt-cnt: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 15/33] counter: microchip-tcb-capture: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 16/33] counter: stm32-lptimer-cnt: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 17/33] counter: stm32-timer-cnt: " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 18/33] counter: Return error code on invalid modes William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 19/33] counter: Standardize to ERANGE for limit exceeded errors William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 20/33] counter: Rename counter_signal_value to counter_signal_level William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 21/33] counter: Rename counter_count_function to counter_function William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 23/33] counter: Update counter.h comments to reflect sysfs internalization William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 24/33] docs: counter: Update " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 25/33] counter: Move counter enums to uapi header William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 26/33] counter: Add character device interface William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 27/33] docs: counter: Document " William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 28/33] tools/counter: Create Counter tools William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 29/33] counter: Implement signalZ_action_component_id sysfs attribute William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 30/33] counter: Implement *_component_id sysfs attributes William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 31/33] counter: Implement events_queue_size sysfs attribute William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 32/33] counter: 104-quad-8: Replace mutex with spinlock William Breathitt Gray
2021-03-09 13:19 ` [PATCH v9 33/33] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
[not found] ` <377a1552e7794ca4d998d840c06ba4a109214863.1615293276.git.vilhelm.gray@gmail.com>
[not found] ` <b195e59c-470b-8a06-7308-42bd628274bd@foss.st.com>
2021-03-14 7:56 ` [Linux-stm32] [PATCH v9 22/33] counter: Internalize sysfs interface code William Breathitt Gray
2021-03-14 9:08 ` William Breathitt Gray
2021-03-18 9:21 ` Fabrice Gasnier
2021-03-18 10:10 ` Fabrice Gasnier [this message]
2021-03-18 14:22 ` William Breathitt Gray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3c3183c4-c9da-709b-dff3-ab5e407e042b@foss.st.com \
--to=fabrice.gasnier@foss.st.com \
--cc=alexandre.belloni@bootlin.com \
--cc=alexandre.torgue@foss.st.com \
--cc=dan.carpenter@oracle.com \
--cc=david@lechnology.com \
--cc=gwendal@chromium.org \
--cc=jic23@kernel.org \
--cc=kamel.bouhara@bootlin.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mcoquelin.stm32@gmail.com \
--cc=o.rempel@pengutronix.de \
--cc=patrick.havelange@essensium.com \
--cc=syednwaris@gmail.com \
--cc=vilhelm.gray@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).