diff mbox series

[v3,2/3] Bluetooth: Add sysfs entry to enable/disable devcoredump

Message ID 20220707151420.v3.2.I39885624992dacff236aed268bdaa69107cd1310@changeid
State New
Headers show
Series [v3,1/3] Bluetooth: Add support for devcoredump | expand

Commit Message

Manish Mandlik July 7, 2022, 10:15 p.m. UTC
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@google.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(+)

Comments

kernel test robot July 8, 2022, 6:52 a.m. UTC | #1
Hi Manish,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth/master]
[also build test WARNING on bluetooth-next/master linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220708-061724
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: i386-randconfig-s002 (https://download.01.org/0day-ci/archive/20220708/202207081448.MghkbBvn-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/0d785cbd11ed3a6de29aeb05926177440ab26d54
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Manish-Mandlik/Bluetooth-Add-support-for-devcoredump/20220708-061724
        git checkout 0d785cbd11ed3a6de29aeb05926177440ab26d54
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash net/bluetooth/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> net/bluetooth/hci_sysfs.c:118:1: sparse: sparse: symbol 'dev_attr_enable_coredump' was not declared. Should it be static?
Luiz Augusto von Dentz July 8, 2022, 5:24 p.m. UTC | #2
Hi Manish,

On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@google.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@google.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.
Luiz Augusto von Dentz July 8, 2022, 6:40 p.m. UTC | #3
Hi Manish,

On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Manish,
>>
>> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@google.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@google.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.

>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> Regards,
> Manish.
Luiz Augusto von Dentz July 8, 2022, 11:18 p.m. UTC | #4
Hi Manish,

On Fri, Jul 8, 2022 at 3:30 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Fri, Jul 8, 2022 at 11:40 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Manish,
>>
>> On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <mmandlik@google.com> wrote:
>> >
>> > Hi Luiz,
>> >
>> > On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> >>
>> >> Hi Manish,
>> >>
>> >> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@google.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@google.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@gmail.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.
diff mbox series

Patch

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)