Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220707151420.v3.1.Iaf638bb9f885f5880ab1b4e7ae2f73dd53a54661@changeid>
 <20220707151420.v3.2.I39885624992dacff236aed268bdaa69107cd1310@changeid>
 <CABBYNZL5oXFsyBN6JRdt1JeD2v5h3MOpCAa4hF0sXfw5F6u3Pg@mail.gmail.com> <CAGPPCLDBVwaXNbr6_wsj-M+Lbdbn6CbZippHYS5F-2vREBPHbg@mail.gmail.com>
In-Reply-To: <CAGPPCLDBVwaXNbr6_wsj-M+Lbdbn6CbZippHYS5F-2vREBPHbg@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 8 Jul 2022 11:40:21 -0700
Message-ID: <CABBYNZLWTbogiUvs3pxE=JPtVX-bxpBa7dKNUL3jh1-a8axR5A@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] Bluetooth: Add sysfs entry to enable/disable devcoredump
To:     Manish Mandlik <mmandlik@google.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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