| 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: <5453A8A6.6020800@opensource.altera.com>
Date: Fri, 31 Oct 2014 10:20:06 -0500
From: Dinh Nguyen <dinguyen@...nsource.altera.com>
To: <balbi@...com>
CC: <paulz@...opsys.com>, <dinh.linux@...il.com>,
<swarren@...dotorg.org>, <b.zolnierkie@...sung.com>,
<matthijs@...in.nl>, <r.baldyga@...sung.com>,
<jg1.han@...sung.com>, <sachin.kamat@...aro.org>,
<ben-linux@...ff.org>, <dianders@...omium.org>,
<kever.yang@...k-chips.com>, <linux-usb@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't
a clock node
On 10/30/2014 09:04 AM, Felipe Balbi wrote:
> Hi,
>
> On Tue, Oct 28, 2014 at 06:25:47PM -0500, dinguyen@...nsource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@...nsource.altera.com>
>>
>> Since the dwc2 hcd driver is currently not looking for a clock node during
>> init, we should not completely fail if there isn't a clock provided.
>> For dual-role mode, we will only fail init for a non-clock node error. We
>> then update the HCD to only call gadget funtions if there is a proper clock
>> node.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@...nsource.altera.com>
>> ---
>> v5: reworked to not access gadget functions from the hcd.
>> ---
>> drivers/usb/dwc2/core.h | 3 +--
>> drivers/usb/dwc2/core_intr.c | 9 ++++++---
>> drivers/usb/dwc2/hcd.c | 3 ++-
>> drivers/usb/dwc2/platform.c | 19 +++++++++++++++----
>> 4 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index ec70862..48120c8 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -660,6 +660,7 @@ struct dwc2_hsotg {
>> #endif
>> #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */
>>
>> + struct clk *clk;
>> #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
>> /* Gadget structures */
>> struct usb_gadget_driver *driver;
>> @@ -667,8 +668,6 @@ struct dwc2_hsotg {
>> struct usb_phy *uphy;
>> struct s3c_hsotg_plat *plat;
>>
>> - struct clk *clk;
>> -
>> struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
>>
>> u32 phyif;
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 1240875..1608037 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>> }
>> /* Change to L0 state */
>> hsotg->lx_state = DWC2_L0;
>> - call_gadget(hsotg, resume);
>> + if (!IS_ERR(hsotg->clk))
>> + call_gadget(hsotg, resume);
>
> instead of exposing the clock detail to the entire driver, add IS_ERR()
> checks to resume and suspend instead. In fact, NULL is a valid clock, so
> you might as well:
>
> clk = clk_get(foo, bar);
> if (IS_ERR(clk))
> dwc->clk = NULL;
> else
> dwc->clk = clk;
>
> Then you don't need any IS_ERR() checks sprinkled around the driver.
But we would still need to check for the clock before accessing gadget
functionality right?
if (dwc2->clk)
call_gadget();
>
>> @@ -400,7 +401,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>> "DSTS.Suspend Status=%d HWCFG4.Power Optimize=%d\n",
>> !!(dsts & DSTS_SUSPSTS),
>> hsotg->hw_params.power_optimized);
>> - call_gadget(hsotg, suspend);
>> + if (!IS_ERR(hsotg->clk))
>> + call_gadget(hsotg, suspend);
>> } else {
>> if (hsotg->op_state == OTG_STATE_A_PERIPHERAL) {
>> dev_dbg(hsotg->dev, "a_peripheral->a_host\n");
>> @@ -477,7 +479,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
>> spin_lock(&hsotg->lock);
>>
>> if (dwc2_is_device_mode(hsotg))
>> - retval = s3c_hsotg_irq(irq, dev);
>> + if (!IS_ERR(hsotg->clk))
>> + retval = s3c_hsotg_irq(irq, dev);
>
> wait a minute, if there is no clock we don't call the gadget interrupt
> handler ? Why ? Who will disable the IRQ line ?
This portion is no longer needed when I use shared IRQ lines.
>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 44c609f..fa49c72 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
>> hsotg->op_state = OTG_STATE_B_PERIPHERAL;
>> dwc2_core_init(hsotg, false, -1);
>> dwc2_enable_global_interrupts(hsotg);
>> - s3c_hsotg_core_init(hsotg);
>> + if (!IS_ERR(hsotg->clk))
>> + s3c_hsotg_core_init(hsotg);
>> } else {
>> /* A-Device connector (Host Mode) */
>> dev_dbg(hsotg->dev, "connId A\n");
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 72f32f7..77c8417 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -217,8 +217,17 @@ static int dwc2_driver_probe(struct platform_device *dev)
>>
>> spin_lock_init(&hsotg->lock);
>> retval = dwc2_gadget_init(hsotg, irq);
>> - if (retval)
>> - return retval;
>> + if (retval) {
>> + /*
>> + * We will not fail the driver initialization for dual-role
>> + * if no clock node is supplied. However, all gadget
>> + * functionality will be disabled if a clock node is not
>> + * provided. Host functionality will continue.
>> + * TO-DO: make clock node a requirement for the HCD.
>> + */
>> + if (!IS_ERR(hsotg->clk))
>> + return retval;
>> + }
>
> no here... this should have been taken care by dwc2_gadget_init()
> itself.
Fixed..
Thanks,
Dinh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists