Date:   Fri, 8 Jul 2022 16:18:42 -0700
From:   Luiz Augusto von Dentz <luiz.dentz@...il.com>
To:     Manish Mandlik <mmandlik@...gle.com>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] Bluetooth: Add sysfs entry to enable/disable devcoredump

Hi Manish,

On Fri, Jul 8, 2022 at 3:30 PM Manish Mandlik <mmandlik@...gle.com> wrote:
>
> Hi Luiz,
>
> On Fri, Jul 8, 2022 at 11:40 AM Luiz Augusto von Dentz <luiz.dentz@...il.com> wrote:
>>
>> Hi Manish,
>>
>> On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <mmandlik@...gle.com> wrote:
>> >
>> > Hi Luiz,
>> >
>> > On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <luiz.dentz@...il.com> wrote:
>> >>
>> >> Hi Manish,
>> >>
>> >> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@...gle.com> wrote:
>> >> >
>> >> > Since device/firmware dump is a debugging feature, we may not need it
>> >> > all the time. Add a sysfs attribute to enable/disable bluetooth
>> >> > devcoredump capturing. The default state of this feature would be
>> >> > disabled and it can be enabled by echoing 1 to enable_coredump sysfs
>> >> > entry as follow:
>> >> >
>> >> > $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump
>> >> >
>> >> > Signed-off-by: Manish Mandlik <mmandlik@...gle.com>
>> >> > ---
>> >> >
>> >> > Changes in v3:
>> >> > - New patch in the series to enable/disable feature via sysfs entry
>> >> >
>> >> >  net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 36 insertions(+)
>> >> >
>> >> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
>> >> > index 4e3e0451b08c..df0d54a5ae3f 100644
>> >> > --- a/net/bluetooth/hci_sysfs.c
>> >> > +++ b/net/bluetooth/hci_sysfs.c
>> >> > @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev)
>> >> >         module_put(THIS_MODULE);
>> >> >  }
>> >> >
>> >> > +#ifdef CONFIG_DEV_COREDUMP
>> >> > +static ssize_t enable_coredump_show(struct device *dev,
>> >> > +                                   struct device_attribute *attr,
>> >> > +                                   char *buf)
>> >> > +{
>> >> > +       struct hci_dev *hdev = to_hci_dev(dev);
>> >> > +
>> >> > +       return scnprintf(buf, 3, "%d\n", hdev->dump.enabled);
>> >> > +}
>> >> > +
>> >> > +static ssize_t enable_coredump_store(struct device *dev,
>> >> > +                                    struct device_attribute *attr,
>> >> > +                                    const char *buf, size_t count)
>> >> > +{
>> >> > +       struct hci_dev *hdev = to_hci_dev(dev);
>> >> > +
>> >> > +       /* Consider any non-zero value as true */
>> >> > +       if (simple_strtol(buf, NULL, 10))
>> >> > +               hdev->dump.enabled = true;
>> >> > +       else
>> >> > +               hdev->dump.enabled = false;
>> >> > +
>> >> > +       return count;
>> >> > +}
>> >> > +DEVICE_ATTR_RW(enable_coredump);
>> >> > +#endif
>> >> > +
>> >> > +static struct attribute *bt_host_attrs[] = {
>> >> > +#ifdef CONFIG_DEV_COREDUMP
>> >> > +       &dev_attr_enable_coredump.attr,
>> >> > +#endif
>> >> > +       NULL,
>> >> > +};
>> >> > +ATTRIBUTE_GROUPS(bt_host);
>> >> > +
>> >> >  static const struct device_type bt_host = {
>> >> >         .name    = "host",
>> >> >         .release = bt_host_release,
>> >> > +       .groups = bt_host_groups,
>> >> >  };
>> >> >
>> >> >  void hci_init_sysfs(struct hci_dev *hdev)
>> >> > --
>> >> > 2.37.0.rc0.161.g10f37bed90-goog
>> >>
>> >> It seems devcoredump.c creates its own sysfs entries so perhaps this
>> >> should be included there and documented in sysfs-devices-coredump.
>> >
>> > I deliberately created it here. We need to have this entry/switch per hci device, whereas the sysfs entry created by devcoredump.c disables the devcoredump feature entirely for anyone who's using it, so it can act as a global switch for the devcoredump.
>>
>> We should probably check if there is a reason why this is not on per
>> device and anyway if seem wrong to each subsystem to come up with its
>> own sysfs entries when it could be easily generalized so the userspace
>> tools using those entries don't have to look into different entries
>> depending on the subsystem the device belongs.
>
> The purpose of the base devcoredump interface is to only provide a generalized mechanism for drivers to dump the firmware data. It is not aware of which subsystem is using it or what data is being dumped. We want to implement something on top of it only for all bluetooth drivers so that they all can generate firmware dumps in a common way and not worry about the synchronizations and timeouts. That's why it made more sense to me to have a bluetooth specific sysfs entry for enabling/disabling only the bluetooth devcoredump interface without affecting the other users of the base devcoredump.

It looks like we are not understanding each other, you are saying
devcoredump only provides a generalized mechanism for the driver to
dump the firmware coredump, yet we are implementing something extra to
generalize it for bluetooth drivers? Making it bluetooth specific sort
of defeat the purpose of a common layer in my opinion and it is not
like the devcoredump.c don't already have entries inside the device
sysfs directory as documented in sysfs-devices-coredump:

What: /sys/devices/.../coredump
Date: December 2017
Contact: Arend van Spriel <aspriel@...il.com>
Description:
The /sys/devices/.../coredump attribute is only present when the
device is bound to a driver, which provides the .coredump()
callback. The attribute is write only. Anything written to this
file will trigger the .coredump() callback.

Available when CONFIG_DEV_COREDUMP is enabled.

What I'm suggesting is in addition to /sys/devices/.../coredump we
also have /sys/devices/.../enable_coredump, perhaps we should include
in the discussion since he is listed as maintainer of DEV_COREDUMP
nowadays.

> Do you suggest we have this sysfs entry for bluetooth class instead? like "/sys/class/bluetooth/enable_coredump" instead of "/sys/class/bluetooth/<device>/enable_coredump"? But the problem with this is that if we have more than one bluetooth controller on the system, disabling one would disable the other as well. So to have the flexibility of controlling it for each device independently I am creating a sysfs entry for each device. IMO, the base devcoredump class sysfs entry is not much of a use anymore as there are already other systems like wifi using it. Let me know your thoughts.
>
>>
>>
>> >
>> >>
>> >> --
>> >> Luiz Augusto von Dentz
>> >
>> >
>> > Regards,
>> > Manish.
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> Regards,
> Manish.



-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists