| 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: <20141103152543.GC27425@saruman>
Date: Mon, 3 Nov 2014 09:25:43 -0600
From: Felipe Balbi <balbi@...com>
To: Dinh Nguyen <dinguyen@...nsource.altera.com>
CC: <balbi@...com>, <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
Hi,
On Fri, Oct 31, 2014 at 02:31:31PM -0500, Dinh Nguyen wrote:
> On 10/31/2014 12:42 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Fri, Oct 31, 2014 at 10:20:06AM -0500, Dinh Nguyen wrote:
> >>>> @@ -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();
> >
> > Read my comment again. "NULL is a valid clock". Look at what
> > clk_enable() does when a NULL pointer is passed:
> >
> > static int __clk_enable(struct clk *clk)
> > {
> > int ret = 0;
> >
> > if (!clk)
> > return 0;
> >
> > if (WARN_ON(clk->prepare_count == 0))
> > return -ESHUTDOWN;
> >
> > if (clk->enable_count == 0) {
> > ret = __clk_enable(clk->parent);
> >
> > if (ret)
> > return ret;
> >
> > if (clk->ops->enable) {
> > ret = clk->ops->enable(clk->hw);
> > if (ret) {
> > __clk_disable(clk->parent);
> > return ret;
> > }
> > }
> > }
> >
> > clk->enable_count++;
> > return 0;
> > }
> >
> > int clk_enable(struct clk *clk)
> > {
> > unsigned long flags;
> > int ret;
> >
> > flags = clk_enable_lock();
> > ret = __clk_enable(clk);
> > clk_enable_unlock(flags);
> >
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(clk_enable);
>
> Ah yes, thanks for the explanation. So if clk=NULL, it just return 0.
> But what I'm saying is that if the driver is configured for dual-role
> mode, and no clock is specified, then the driver should not be accessing
> any gadget functionality.
why ? Why only for gadget and why can't it work on platforms without
clk? What if Paul wants to run the gadget side on his HAPS-5x platform
configured as a PCIe card ?
You haven't explained why gadget has this hard-dependency on clk and why
*only* gadget has it. This sounds really, really wrong. Why can host
side run without clk but gadget can't ?
Moreover, if you really want to prevent people from using gadget
without clock, fail dwc2_gadget_init() and have the core fallback to
host-only, then print a warning message.
> So as the patch series stands right now, if I swap out an A connector to
> a B-connector, then I get a connect_id_status change interrupt. The
> status would show a device and I would initialize the gadget portion of
> the driver.
that's fine, but why the hard-dependency on clk ?
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 44c609f..96810f7 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 (hsotg->clk)
> + s3c_hsotg_core_init(hsotg);
>
> So if I don't have a valid clock, I'll be accessing the peripheral
> portion of the IP.
so what ?
> But I guess not having the check for the valid clock here should be fine
> as I don't see a case where there can be 2 different clocks for host and
> peripheral?
probably the same thing.
> >>>> @@ -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 static int __clk_enable(struct clk *clk)
> >
> > huh ? What I mean is that this has the potential of leaving that IRQ
> > line enabled. Imagine you don't have a clock and s3c_hsotg_irq() isn't
> > called, then a peripheral IRQ fires, since the handler isn't called, who
> > will clear the interrupt ?
> >
>
> Yes, right. This portion of the code is no longer needed when I switched
> to use a shared IRQ.
k
--
balbi
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists