linux-kernel - RE: [RFC PATCH v1 13/14] usb:cdns3: Adds debugging function.

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

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

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

Date:   Mon, 5 Nov 2018 06:17:51 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Joe Perches <joe@...ches.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "rogerq@...com" <rogerq@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Alan Douglas <adouglas@...ence.com>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "peter.chen@....com" <peter.chen@....com>,
        Pawel Jez <pjez@...ence.com>, Rahul Kumar <kurahul@...ence.com>
Subject: RE: [RFC PATCH v1 13/14] usb:cdns3: Adds debugging function.

>> Patch implements some function used for debugging driver.
>[]
>> +static inline char *cdns3_decode_ep_irq(u32 ep_sts, const char *ep_name)
>> +{
>> +	static char str[256];
>> +	int ret;
>> +
>> +	ret = sprintf(str, "IRQ for %s: %08x ", ep_name, ep_sts);
>> +
>> +	if (ep_sts & EP_STS_SETUP)
>> +		ret += sprintf(str + ret, "SETUP ");
>> +	if (ep_sts & EP_STS_IOC)
>> +		ret += sprintf(str + ret, "IOC ");
>> +	if (ep_sts & EP_STS_ISP)
>> +		ret += sprintf(str + ret, "ISP ");
>> +	if (ep_sts & EP_STS_DESCMIS)
>> +		ret += sprintf(str + ret, "DESCMIS ");
>> +	if (ep_sts & EP_STS_STREAMR)
>> +		ret += sprintf(str + ret, "STREAMR ");
>> +	if (ep_sts & EP_STS_MD_EXIT)
>> +		ret += sprintf(str + ret, "MD_EXIT ");
>> +	if (ep_sts & EP_STS_TRBERR)
>> +		ret += sprintf(str + ret, "TRBERR ");
>> +	if (ep_sts & EP_STS_NRDY)
>> +		ret += sprintf(str + ret, "NRDY ");
>> +	if (ep_sts & EP_STS_PRIME)
>> +		ret += sprintf(str + ret, "PRIME ");
>> +	if (ep_sts & EP_STS_SIDERR)
>> +		ret += sprintf(str + ret, "SIDERRT ");
>> +	if (ep_sts & EP_STS_OUTSMM)
>> +		ret += sprintf(str + ret, "OUTSMM ");
>> +	if (ep_sts & EP_STS_ISOERR)
>> +		ret += sprintf(str + ret, "ISOERR ");
>> +	if (ep_sts & EP_STS_IOT)
>> +		ret += sprintf(str + ret, "IOT ");
>> +
>> +	return str;
>> +}
>
>This bit is pretty unsightly.
>Especially the static in each inline

Hi Joe.

I understood that you mean line 
static char str[256];
This array will be defined several times. 

I will remove inline form function definition. 
It's not necessary. 

>> +
>> +char *cdns3_decode_epx_irq(struct cdns3_endpoint *priv_ep)
>> +{
>> +	struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
>> +
>> +	return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
>> +				   priv_ep->name);
>> +}
>> +
>> +char *cdns3_decode_ep0_irq(struct cdns3_device *priv_dev, int dir)
>> +{
>> +	if (dir)
>> +		return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
>> +					   "ep0IN");
>> +	else
>> +		return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
>> +					   "ep0OUT");
>> +}
>> +
>[]
>> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
>[]
>> @@ -604,12 +604,15 @@ void cdns3_check_ep0_interrupt_proceed(struct cdns3_device *priv_dev, int dir)
>>  	cdns3_select_ep(priv_dev, 0 | (dir ? USB_DIR_IN : USB_DIR_OUT));
>>  	ep_sts_reg = readl(&regs->ep_sts);
>>
>> +	dev_dbg(&priv_dev->dev, "%s\n", cdns3_decode_ep0_irq(priv_dev, dir));
>[]
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>[]
>> @@ -537,6 +547,8 @@ static int cdns3_check_ep_interrupt_proceed(struct cdns3_endpoint *priv_ep)
>>  	cdns3_select_ep(priv_dev, priv_ep->endpoint.address);
>>  	ep_sts_reg = readl(&regs->ep_sts);
>>
>> +	dev_dbg(&priv_dev->dev, "%s\n", cdns3_decode_epx_irq(priv_ep));
>>

Thank you for comment.
Cheers Pawel