| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1d69cef1-400d-3841-982f-cd37964bfd06@ti.com>
Date: Mon, 6 Aug 2018 13:33:14 +0300
From: Roger Quadros <rogerq@...com>
To: Pawel Laszczak <pawell@...ence.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
Felipe Balbi <balbi@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Lukasz Tyrala <ltyrala@...ence.com>,
Alan Douglas <adouglas@...ence.com>
Subject: Re: [PATCH 05/31] usb: usbssp: Added first part of initialization
sequence.
On 06/08/18 11:57, Pawel Laszczak wrote:
> Hi,
>>
>> Hi,
>>
>> On 19/07/18 20:57, Pawel Laszczak wrote:
>>> Patch adds some initialization function. The initialization sequence
>>> is quite complicated and this patch implements it only partially.
>>> Initialization will be completed in next few patches.
>>>
>>> Patch introduce three new files:
>>> 1. gadget-dbg.c - file contains functions used for debugging purpose.
>>> 2. gadget-ext-caps.h - holds macro definition related to
>>> Extended Capabilities
>>> 3. gadget-if - file implements stuff related to upper layer
>>> (e.g usb_ep_ops, usb_gadget_ops interface).
>>>
>>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>>> ---
>>> drivers/usb/usbssp/Makefile | 1 +
>>> drivers/usb/usbssp/gadget-dbg.c | 30 ++++
>>> drivers/usb/usbssp/gadget-ext-caps.h | 53 ++++++
>>> drivers/usb/usbssp/gadget-if.c | 24 +++
>>> drivers/usb/usbssp/gadget.c | 242 +++++++++++++++++++++++++++
>>> drivers/usb/usbssp/gadget.h | 15 ++
>>> 6 files changed, 365 insertions(+)
>>> create mode 100644 drivers/usb/usbssp/gadget-dbg.c
>>> create mode 100644 drivers/usb/usbssp/gadget-ext-caps.h
>>> create mode 100644 drivers/usb/usbssp/gadget-if.c
>>>
>>> diff --git a/drivers/usb/usbssp/Makefile b/drivers/usb/usbssp/Makefile
>>> index d85f15afb51c..0606f3c63cd0 100644
>>> --- a/drivers/usb/usbssp/Makefile
>>> +++ b/drivers/usb/usbssp/Makefile
>>> @@ -5,6 +5,7 @@ CFLAGS_gadget-trace.o := -I$(src)
>>> obj-$(CONFIG_USB_USBSSP_GADGET) += usbssp.o
>>> usbssp-y := usbssp-plat.o gadget-ring.o \
>>> gadget.o
>>> + gadget-dbg.o
>>>
>>> ifneq ($(CONFIG_TRACING),)
>>> usbssp-y += gadget-trace.o
>>> diff --git a/drivers/usb/usbssp/gadget-dbg.c b/drivers/usb/usbssp/gadget-dbg.c
>>> new file mode 100644
>>> index 000000000000..277617d1a996
>>> --- /dev/null
>>> +++ b/drivers/usb/usbssp/gadget-dbg.c
>>> @@ -0,0 +1,30 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * USBSSP device controller driver
>>> + *
>>> + * Copyright (C) 2018 Cadence.
>>> + *
>>> + * Author: Pawel Laszczak
>>> + *
>>> + * A lot of code based on Linux XHCI driver.
>>> + * Origin: Copyright (C) 2008 Intel Corp
>>> + */
>>> +
>>> +#include "gadget.h"
>>> +
>>> +void usbssp_dbg_trace(struct usbssp_udc *usbssp_data,
>>> + void (*trace)(struct va_format *),
>>> + const char *fmt, ...)
>>> +{
>>> + struct va_format vaf;
>>> + va_list args;
>>> +
>>> + va_start(args, fmt);
>>> + vaf.fmt = fmt;
>>> + vaf.va = &args;
>>> + dev_dbg(usbssp_data->dev, "%pV\n", &vaf);
>>
>> dev_dbg will mess up with timings to be useful.
>> Why not just use one of the trace events you defined in gadget-trace.h?
>
> Function is equivalent of xhci_dbg_trace from XHCI driver.
> It is a function of general use.
> The problem with trace is that trace log is deleted after rebooting system.
> Using trace log is problematic during developing and debugging driver.
OK. But I think trace log is more useful for I/O related debugging as it
keeps the timing impact minimal. For things that don't come in the I/O path
you could use dev_dbg().
> Do you know whether trace log is saved anywhere on disk ?
>
I think trace_cmd allows you to record on the disk or even send it over the network.
https://lwn.net/Articles/410200/
>>> + trace(&vaf);
>>> + va_end(args);
>>> +}
>>> +EXPORT_SYMBOL_GPL(usbssp_dbg_trace);
>>> +
<snip>
>>> int usbssp_gadget_init(struct usbssp_udc *usbssp_data)
>>> {
>>> int ret;
>>> +
>>> + /*
>>> + * Check the compiler generated sizes of structures that must be laid
>>> + * out in specific ways for hardware access.
>>> + */
>>> + BUILD_BUG_ON(sizeof(struct usbssp_doorbell_array) != 2*32/8);
>>> + BUILD_BUG_ON(sizeof(struct usbssp_slot_ctx) != 8*32/8);
>>> + BUILD_BUG_ON(sizeof(struct usbssp_ep_ctx) != 8*32/8);
>>> + /* usbssp_device has eight fields, and also
>>> + * embeds one usbssp_slot_ctx and 31 usbssp_ep_ctx
>>> + */
>>> + BUILD_BUG_ON(sizeof(struct usbssp_stream_ctx) != 4*32/8);
>>> + BUILD_BUG_ON(sizeof(union usbssp_trb) != 4*32/8);
>>> + BUILD_BUG_ON(sizeof(struct usbssp_erst_entry) != 4*32/8);
>>> + BUILD_BUG_ON(sizeof(struct usbssp_cap_regs) != 8*32/8);
>>> + BUILD_BUG_ON(sizeof(struct usbssp_intr_reg) != 8*32/8);
>>> + /* usbssp_run_regs has eight fields and embeds 128 usbssp_intr_regs */
>>> + BUILD_BUG_ON(sizeof(struct usbssp_run_regs) != (8+8*128)*32/8);
>>> +
>>> + /* fill gadget fields */
>>> + /*TODO: implements usbssp_gadget_ops object*/
>>> + //usbssp_data->gadget.ops = &usbssp_gadget_ops;
>>> + usbssp_data->gadget.name = "usbssp-gadget";
>>> + usbssp_data->gadget.max_speed = USB_SPEED_SUPER_PLUS;
>>> + usbssp_data->gadget.speed = USB_SPEED_UNKNOWN;
>>> + usbssp_data->gadget.sg_supported = true;
>>> + usbssp_data->gadget.lpm_capable = 1;
>>> +
>>> + usbssp_data->setup_buf = kzalloc(USBSSP_EP0_SETUP_SIZE, GFP_KERNEL);
>>> + if (!usbssp_data->setup_buf)
>>> + return -ENOMEM;
>>> +
>>> + /*USBSSP support not aligned buffer but this option
>>> + * improve performance of this controller.
>>> + */
>>
>> Multi-line comment formatting. Checkpach should complain.
> It doesn't complain about this case. I don't know why.
>
Can you try with --strict option to checkpatch?
>>> + usbssp_data->gadget.quirk_ep_out_aligned_size = true;
>>> + ret = usbssp_gen_setup(usbssp_data);
>>> + if (ret < 0) {
>>> + dev_err(usbssp_data->dev,
>>> + "Generic initialization failed with error code%d\n",
>>> + ret);
>>> + goto err3;
>>> + }
>>> +
>>> + ret = usbssp_gadget_init_endpoint(usbssp_data);
>>> + if (ret < 0) {
>>> + dev_err(usbssp_data->dev, "failed to initialize endpoints\n");
>>> + goto err1;
>>> + }
>>> +
>>> + ret = usb_add_gadget_udc(usbssp_data->dev, &usbssp_data->gadget);
>>> +
>>> + if (ret) {
>>> + dev_err(usbssp_data->dev, "failed to register udc\n");
>>> + goto err2;
>>> + }
>>> +
>>> + return ret;
>>> +err2:
>>> + usbssp_gadget_free_endpoint(usbssp_data);
>>> +err1:
>>> + usbssp_halt(usbssp_data);
>>> + /*TODO add implementation of usbssp_reset function*/
>>> + //usbssp_reset(usbssp_data);
>>> + /*TODO add implementation of freeing memory*/
>>> + //usbssp_mem_cleanup(usbssp_data);
>>> +err3:
>>> return ret;
>>> }
>>>
>>> @@ -60,5 +298,9 @@ int usbssp_gadget_exit(struct usbssp_udc *usbssp_data)
>>> {
>>> int ret = 0;
>>>
>>> + usb_del_gadget_udc(&usbssp_data->gadget);
>>> + usbssp_gadget_free_endpoint(usbssp_data);
>>> + /*TODO: add usbssp_stop implementation*/
>>> + //usbssp_stop(usbssp_data);
>>> return ret;
>>> }
>>> diff --git a/drivers/usb/usbssp/gadget.h b/drivers/usb/usbssp/gadget.h
>>> index 55e20795d900..5d8918f8da84 100644
>>> --- a/drivers/usb/usbssp/gadget.h
>>> +++ b/drivers/usb/usbssp/gadget.h
>>> @@ -12,8 +12,10 @@
>>> #ifndef __LINUX_USBSSP_GADGET_H
>>> #define __LINUX_USBSSP_GADGET_H
>>>
>>> +#include <linux/irq.h>
>>> #include <linux/io-64-nonatomic-lo-hi.h>
>>> #include <linux/usb/gadget.h>
>>> +#include "gadget-ext-caps.h"
>>>
>>> /* Max number slots - only 1 is allowed */
>>> #define DEV_MAX_SLOTS 1
>>> @@ -1676,7 +1678,18 @@ static inline void usbssp_write_64(struct usbssp_udc *usbssp_data,
>>> lo_hi_writeq(val, regs);
>>> }
>>>
>>> +/* USBSSP memory management */
>>> +void usbssp_dbg_trace(struct usbssp_udc *usbssp_data,
>>> + void (*trace)(struct va_format *),
>>> + const char *fmt, ...);
>>
>> what has this function to do with memory management?
>>
> Nothing,
>
>>> /* USBSSP Device controller glue */
>>> +void usbssp_bottom_irq(struct work_struct *work);
>>
>> usbssp_bottom_irq() wasn't defined in this patch.
> Removed from this patch
>
>>> +int usbssp_init(struct usbssp_udc *usbssp_data);
>>> +void usbssp_stop(struct usbssp_udc *usbssp_data);
>>> +int usbssp_handshake(void __iomem *ptr, u32 mask, u32 done, int usec);
>>> +void usbssp_quiesce(struct usbssp_udc *usbssp_data);
>>> +extern int usbssp_reset(struct usbssp_udc *usbssp_data);
>>> +
>>> int usbssp_suspend(struct usbssp_udc *usbssp_data, bool do_wakeup);
>>> int usbssp_resume(struct usbssp_udc *usbssp_data, bool hibernated);
>>>
>>> @@ -1689,6 +1702,8 @@ dma_addr_t usbssp_trb_virt_to_dma(struct usbssp_segment *seg,
>>> /* USBSSP gadget interface*/
>>> int usbssp_gadget_init(struct usbssp_udc *usbssp_data);
>>> int usbssp_gadget_exit(struct usbssp_udc *usbssp_data);
>>> +void usbssp_gadget_free_endpoint(struct usbssp_udc *usbssp_data);
>>> +int usbssp_gadget_init_endpoint(struct usbssp_udc *usbssp_data);
>>>
>>> static inline char *usbssp_slot_state_string(u32 state)
>>> {
>>>
>>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Powered by blists - more mailing lists