linux-kernel - Re: [PATCH v4 2/2] Embedded USB Debugger (EUD) driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Feb 2020 12:51:52 +0530
From:   "Dwivedi, Avaneesh Kumar (avani)" <akdwived@...eaurora.org>
To:     Greg KH <greg@...ah.com>
Cc:     linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        ckadabi@...eaurora.org, tsoni@...eaurora.org,
        bryanh@...eaurora.org, psodagud@...eaurora.org,
        rnayak@...eaurora.org, satyap@...eaurora.org,
        pheragu@...eaurora.org
Subject: Re: [PATCH v4 2/2] Embedded USB Debugger (EUD) driver


On 2/16/2020 10:05 PM, Greg KH wrote:
> On Sun, Feb 16, 2020 at 09:52:19PM +0530, Dwivedi, Avaneesh Kumar (avani) wrote:
>> On 2/7/2020 3:34 PM, Greg KH wrote:
>>> On Fri, Jan 31, 2020 at 10:13:31AM +0530, Avaneesh Kumar Dwivedi wrote:
>>>> Add support for control peripheral of EUD (Embedded USB Debugger) to
>>>> listen to events such as USB attach/detach, charger enable/disable, pet
>>>> EUD to indicate software is functional. Reusing the platform device kobj,
>>>> sysfs entry 'enable' is created to enable or disable EUD.
>>>>
>>>> Signed-off-by: Satya Durga Srinivasu Prabhala <satyap@...eaurora.org>
>>>> Signed-off-by: Prakruthi Deepak Heragu <pheragu@...eaurora.org>
>>>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@...eaurora.org>
>>>> ---
>>>>    Documentation/ABI/stable/sysfs-driver-msm-eud |   5 +
>>>>    drivers/soc/qcom/Kconfig                      |  12 +
>>>>    drivers/soc/qcom/Makefile                     |   1 +
>>>>    drivers/soc/qcom/eud.c                        | 329 ++++++++++++++++++++++++++
>>>>    4 files changed, 347 insertions(+)
>>>>    create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
>>>>    create mode 100644 drivers/soc/qcom/eud.c
>>>>
>>>> diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
>>>> new file mode 100644
>>>> index 0000000..d96ae05
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
>>>> @@ -0,0 +1,5 @@
>>>> +What:           /sys/bus/platform/drivers/msm-eud/enable
>>>> +Date:           Jan 2020
>>>> +Contact:        Avaneesh Kumar Dwivedi <akdwived@...eaurora.org>
>>>> +Description:    Enable/Disable use of eud device.
>>> What are valid values to be used here?
>> it should be bool variable relying on 0 or 1.
> Then document it.
OK
>
>>>> +Users:          User space debug application which intend to use EUD h/w block.
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index d0a73e7..6b7c9d0 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -202,4 +202,16 @@ config QCOM_APR
>>>>    	  application processor and QDSP6. APR is
>>>>    	  used by audio driver to configure QDSP6
>>>>    	  ASM, ADM and AFE modules.
>>>> +
>>>> +config QCOM_EUD
>>>> +       tristate "QTI Embedded USB Debugger (EUD)"
>>>> +       depends on ARCH_QCOM
>>> Why not let everyone test build this?
>> EUD is Qualcomm IP, shall not it be associated with ARCH_QCOM?
> No, why can't everyone buid it for testing?  What about when I want to
> build a generic arm64 kernel to run on multiple SoCs?
>
> Do not put dependancies in here that you really do not have.  There's no
> reason for this to be limited to that one chip, right?  And if you allow
> others to build the code, you will get proper bug reports when things
> break, and others will fix them, which is what you want.
>
> I think the ARCH_RANDOM_SOC_NAME is totally broken and needs to be, at
> most, just an arch-specific thing, if even that.
>
> Look at almost all other kernel drivers, they do not have those types of
> dependancies.
Will check and address concerns.
>
>>>> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>>> +	if (!chip)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	chip->dev = &pdev->dev;
>>> No reference counting???
>> you mean get/put_device?
> yes.
>
> thanks,
>
> greg k-h
Thank you very much Greg for your time to review.

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.