On 25-02-2019 17:45, Thierry Reding wrote:
> On Thu, Feb 14, 2019 at 08:22:34PM +0530, Nagarjuna Kristam wrote:
>>
>>
>>>> #
>>>> +static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc)
>>>> +{
>>>> + unsigned long flags;
>>>> + int err;
>>>> +
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + if (xudc->device_mode) {
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>> + return;
>>>> + }
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>
>>> I'm not sure I understand what you're trying to achive here. This
>>> locking doesn't buy you anything at all. First, you're not protecting a
>>> race condition here since you're not modifying xudc->device_mode in the
>>> critical section.
>>>
>>> Once you unlock here, xudc->device_mode could be modified while you're
>>> executing the rest of the code, exposing you to a potential race
>>> condition. So either you already have proper locking in place and can
>>> drop the locking from here, or you need to make sure to keep the lock
>>> until you are in device mode and set xudc->device_mode later on.
>>>
>>
>> reason for split spin_lock usage is to avoid creating dead lock,
>> where pm_runtime_get_sync also takes the same lock.
>> Will re-check and update the logic accordingly, maybe use mutex to syncronize
>> device_mode on and off functionality.
>
> As-is, the above doesn't make any sense because you lock, read a
> variable and immediately unlock again, irrespective of what the value
> was and without doing anything else in between. Reading the variable is
> already an atomic operation, meaning you'll read either true or false.
> There's no potential for any race conditions in the above, so the
> locking here is not needed.
>
using mutex lock, can syncronize runtime call and device mode on/off calls
this also helps to get rid of WARN added in suspend.
>>>> +
>>>> + pm_runtime_get_sync(xudc->dev);
>>>> +
>>>> + err = phy_power_on(xudc->utmi_phy);
>>>> + if (err < 0)
>>>> + dev_err(xudc->dev, "utmi power on failed %d\n", err);
>>>> + err = phy_power_on(xudc->usb3_phy);
>>>> + if (err < 0)
>>>> + dev_err(xudc->dev, "usb3 phy power on failed %d\n", err);
>>>> +
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + dev_info(xudc->dev, "device mode on\n");
>>>
>>> dev_dbg()?
>>>
>>>> + tegra_xusb_padctl_set_vbus_override(xudc->padctl);
>>>
>>> What exactly does that do here? Could we automatically run this as part
>>> of phy_power_on(xudc->utmi_phy)? Perhaps we need to combine this with a
>>> phy_configure() call that can be used to configure the PHY into device
>>> mode? Based on the mode, phy_power_on() could then set or clear the VBUS
>>> override.
>>>
>>> Or I suppose phy_configure() itself could set or clear the VBUS
>>> override if the sequencing is important.
>>>
>>
>> As discussed over other patch discussion, we will keep vbus override functionality
>>
>>>> +
>>>> + xudc->device_mode = true;
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>> + tegra_phy_xusb_utmi_pad_power_on(xudc->utmi_phy);
>>>
>>> The way this is used I'm wondering if this shouldn't be part of the
>>> xudc->utmi_phy ->power_on() implementation.
>>>
>>
>> Will handle as part of phy_power_on
>>
>>>> +}
>>>> +
>>>> +static void tegra_xudc_device_mode_off(struct tegra_xudc *xudc)
>>>> +{
>>>> + bool connected = false;
>>>> + unsigned long flags;
>>>> + u32 pls, val;
>>>> + int err;
>>>> +
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + if (!xudc->device_mode) {
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>> + return;
>>>> + }
>>>> +
>>>> + dev_info(xudc->dev, "device mode off\n");
>>>
>>> dev_dbg()
>>>
>>
>> Marked info to get run time update on mode changes.
>
> Aren't there any other mechanisms to check mode changes? I would expect
> there to be files in sysfs or debugfs that would allow one to read the
> mode out of.
>
> As a general rule, only output things that are of significance to the
> user. In this particular case, it's reasonable for the user to expect a
> mode change to just work, in which case there's no use in informing the
> user. If you want to leave this in for debugging, that's fine, but then
> please turn this into dev_dbg() so that it is only shown when needed,
> which is while debugging.
>
Will keep them at dbg level
>>>> +
>>>> + connected = !!(xudc_readl(xudc, PORTSC) & PORTSC_CCS);
>>>> + reinit_completion(&xudc->disconnect_complete);
>>>> +
>>>> + tegra_xusb_padctl_clear_vbus_override(xudc->padctl);
>>>> +
>>>> + pls = (xudc_readl(xudc, PORTSC) >> PORTSC_PLS_SHIFT) &
>>>> + PORTSC_PLS_MASK;
>>>> +
>>>> + /* Direct link to U0 if disconnected in RESUME or U2. */
>>>> + if (xudc->soc->pls_quirk && xudc->gadget.speed == USB_SPEED_SUPER &&
>>>> + (pls == PORTSC_PLS_RESUME || pls == PORTSC_PLS_U2)) {
>>>> + val = xudc_readl(xudc, PORTPM);
>>>> + val |= PORTPM_FRWE;
>>>> + xudc_writel(xudc, val, PORTPM);
>>>> +
>>>> + val = xudc_readl(xudc, PORTSC);
>>>> + val &= ~(PORTSC_CHANGE_MASK |
>>>> + (PORTSC_PLS_MASK << PORTSC_PLS_SHIFT));
>>>> + val |= PORTSC_LWS | (PORTSC_PLS_U0 << PORTSC_PLS_SHIFT);
>>>> + xudc_writel(xudc, val, PORTSC);
>>>> + }
>>>> +
>>>> + xudc->device_mode = false;
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>
>>> The locking is again somewhat weird here. Technically someone else could
>>> jump right in at this point and reconfigure to device mode again. Would
>>> that not interfere with the operations below?
>>>
>>
>> As mentioned above, will check possibility to protect this using a mutex instead.
>>
>>>> + tegra_phy_xusb_utmi_pad_power_down(xudc->utmi_phy);
>>>
>>> Again, if sequencing isn't crucial here, couldn't this be moved into
>>> phy_power_off()?
>>>
>>>> +
>>>> + /* Wait for disconnect event. */
>>>> + if (connected)
>>>> + wait_for_completion(&xudc->disconnect_complete);
>>>> +
>>>> + /* Make sure interrupt handler has completed before powergating. */
>>>> + synchronize_irq(xudc->irq);
>>>> + err = phy_power_off(xudc->utmi_phy);
>>>> + if (err < 0)
>>>> + dev_err(xudc->dev, "utmi_phy power off failed %d\n", err);
>>>> +
>>>> + err = phy_power_off(xudc->usb3_phy);
>>>> + if (err < 0)
>>>> + dev_err(xudc->dev, "usb3_phy power off failed %d\n", err);
>>>> +
>>>> + pm_runtime_put(xudc->dev);
>>>> +}
>>>
>>> Also, you seem to be using phy_power_on() and phy_power_off()
>>> symmetrically with pm_runtime_get() and pm_runtime_put(), so I'm
>>> wondering if phy_power_on() and phy_power_off() can't be moved into the
>>> runtime PM callbacks.
>>>
>>
>> phy_power is coupled with device_mode on/off sequence and runtime_get and put
>> are intermittently called along with gadget driver callbacks also and hence they
>> are separated out
>
> Is there any reason to keep a runtime PM reference if we disable device
> mode? I'm not sure I understand why PHY and runtime PM are separated
> here.
>
> If PHY power on/off are called in other places, does it perhaps make
> sense to call runtime PM get/put in those same places as well? Or are
> there any reasons to keep XUDC enabled while the PHY is off?
>
> If so, it's fine to keep these separate.
>
Gadget Driver can call gadget start and stop based on drivers been registered,
These start stop sequence needs access to xudc registers and hence they call runtime
API's to toggle power when phy is off(no cable attached), hence phy and runtime
are separated out for these specific functions
>>>> +
>>>> +static void tegra_xudc_update_data_role(struct tegra_xudc *xudc)
>>>> +{
>>>> + if (extcon_get_state(xudc->data_role_extcon, EXTCON_USB))
>>>> + tegra_xudc_device_mode_on(xudc);
>>>> + else
>>>> + tegra_xudc_device_mode_off(xudc);
>>>> +}
>>>> +
>>>> +static void tegra_xudc_data_role_work(struct work_struct *work)
>>>> +{
>>>> + struct tegra_xudc *xudc = container_of(work, struct tegra_xudc,
>>>> + data_role_work);
>>>> +
>>>> + tegra_xudc_update_data_role(xudc);
>>>> +}
>>>> +
>>>> +static int tegra_xudc_data_role_notifier(struct notifier_block *nb,
>>>> + unsigned long event, void *unused)
>>>> +{
>>>> + struct tegra_xudc *xudc = container_of(nb, struct tegra_xudc,
>>>> + data_role_nb);
>>>> + unsigned long flags;
>>>> +
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + if (!xudc->suspended)
>>>> + schedule_work(&xudc->data_role_work);
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>> +
>>>> + return NOTIFY_DONE;
>>>> +}
>>>> +
>>>> +static void tegra_xudc_plc_reset_work(struct work_struct *work)
>>>> +{
>>>> + struct delayed_work *dwork = to_delayed_work(work);
>>>> + struct tegra_xudc *xudc = container_of(dwork, struct tegra_xudc,
>>>> + plc_reset_work);
>>>> + unsigned long flags;
>>>> +
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + if (xudc->wait_csc) {
>>>> + u32 pls = (xudc_readl(xudc, PORTSC) >> PORTSC_PLS_SHIFT) &
>>>> + PORTSC_PLS_MASK;
>>>> + if (pls == PORTSC_PLS_INACTIVE) {
>>>> + dev_info(xudc->dev, "PLS = Inactive. Toggle VBUS\n");
>>>> + tegra_xusb_padctl_clear_vbus_override(xudc->padctl);
>>>> + tegra_xusb_padctl_set_vbus_override(xudc->padctl);
>>>
>>> Isn't this the same as the reset quirk?
>>>
>> Both plc reset and port reset quirk are triggered on different port status.
>> Even though they need to toggle vbus, conditions on which they do have different
>> conditions and hence separated out
>
> Okay.
>
>>
>>>> + xudc->wait_csc = false;
>>>> + }
>>>> + }
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>> +}
>>>> +
>>>> +static void tegra_xudc_port_reset_war_work(struct work_struct *work)
>>>> +{
>>>> + struct delayed_work *dwork = to_delayed_work(work);
>>>> + struct tegra_xudc *xudc =
>>>> + container_of(dwork, struct tegra_xudc, port_reset_war_work);
>>>> + unsigned long flags;
>>>> + u32 pls;
>>>> + int ret;
>>>> +
>>>> + dev_info(xudc->dev, "port_reset_war_work\n");
>>>
>>> dev_dbg(), though you may also want to reword this to be more useful.
>>>
>> Will udpate
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + if (xudc->data_role_extcon &&
>>>> + extcon_get_state(xudc->data_role_extcon, EXTCON_USB) &&
>>>> + xudc->wait_for_sec_prc) {
>>>
>>> This could use a local variable to make the conditional easier to read.
>>>
>> Will udpate
>>>> + pls = (xudc_readl(xudc, PORTSC) >> PORTSC_PLS_SHIFT) &
>>>> + PORTSC_PLS_MASK;
>>>> + dev_info(xudc->dev, "pls = %x\n", pls);
>>>
>>> dev_dbg()
>>>
>> Will udpate
>>>> +
>>>> + if (pls == PORTSC_PLS_DISABLED) {
>>>> + dev_info(xudc->dev, "toggle vbus\n");
>>>
>>> dev_dbg()
>>>
>> Will udpate
>>>> + /* PRC doesn't complete in 100ms, toggle the vbus */
>>>> + ret =
>>>> + tegra_phy_xusb_utmi_port_reset_quirk(xudc->utmi_phy);
>>>
>>> Would phy_reset() work in this case? That would have the nice
>>> side-effect of making the expression short enough for it all to fit on
>>> one line.
>>>
>>
>> As discussed earlier, phy_reset and port reset need to be separated out
>
> Okay then. Perhaps we can leave out _quirk from the function name to
> make this somewhat shorter? The way the above is wrapped makes my eyes
> hurt. =)
>
Will remove quick and keep it port_reset
> [...]
>>>> +static int
>>>> +tegra_xudc_ep_queue(struct usb_ep *usb_ep, struct usb_request *usb_req,
>>>> + gfp_t gfp)
>>>> +{
>>>> + struct tegra_xudc_request *req;
>>>> + struct tegra_xudc_ep *ep;
>>>> + struct tegra_xudc *xudc;
>>>> + unsigned long flags;
>>>> + int ret;
>>>> +
>>>> + if (!usb_ep || !usb_req)
>>>> + return -EINVAL;
>>>> + ep = to_xudc_ep(usb_ep);
>>>> + req = to_xudc_req(usb_req);
>>>> + xudc = ep->xudc;
>>>> +
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + if (xudc->powergated || !ep->desc) {
>>>
>>> Does this even happen? Shouldn't we make sure that this can never be
>>> called when powergated?
>>>
>>
>> During fast plug and unplug, In the disconnect path,
>> there can be a race between XUDC entering runtime suspend
>> and the gadget core resetting (unbinding and re-binding), resulting in this access.
>
> Okay, sounds fine then.
>
> [...]
>>>> + ep_reload(xudc, ep->index);
>>>> + ep_unpause(xudc, ep->index);
>>>> + ep_unhalt(xudc, ep->index);
>>>> +
>>>> + if (usb_endpoint_xfer_isoc(desc)) {
>>>> + for (i = 0; i < ARRAY_SIZE(xudc->ep); i++) {
>>>> + if (xudc->ep[i].desc &&
>>>> + usb_endpoint_xfer_bulk(xudc->ep[i].desc))
>>>> + ep_unpause(xudc, i);
>>>> + }
>>>> + }
>>>> +
>>>> +out:
>>>> + dev_info(xudc->dev, "ep %u (type: %d, dir: %s) enabled\n", ep->index,
>>>> + usb_endpoint_type(ep->desc),
>>>> + usb_endpoint_dir_in(ep->desc) ? "in" : "out");
>>>
>>> dev_dbg()
>>>
>>
>> Maked info, to get run time udpated on enabled end points
>
> Again, can this information not be obtained elsewhere? The kernel log is
> a precious tool to quickly see when unexpected things happen. If we keep
> spamming the log with information that's not useful, we make it more
> difficult to see the really important bits.
>
device mode on off could be identified by looking at sys node based on gadget
state, but which end points are enabled are run time. Even this indirectly identified
based on which functions are loaded, but what EP's of functions is not found
from sys interface.
> [...]
>>>> +static int tegra_xudc_ep0_enable(struct usb_ep *usb_ep,
>>>> + const struct usb_endpoint_descriptor *desc)
>>>> +{
>>>> + return -EINVAL;
>>>> +}
>>>> +
>>>> +static int tegra_xudc_ep0_disable(struct usb_ep *usb_ep)
>>>> +{
>>>> + return -EINVAL;
>>>> +}
>>>> +
>>>> +static struct usb_ep_ops tegra_xudc_ep0_ops = {
>>>> + .enable = tegra_xudc_ep0_enable,
>>>> + .disable = tegra_xudc_ep0_disable,
>>>> + .alloc_request = tegra_xudc_ep_alloc_request,
>>>> + .free_request = tegra_xudc_ep_free_request,
>>>> + .queue = tegra_xudc_ep_queue,
>>>> + .dequeue = tegra_xudc_ep_dequeue,
>>>> + .set_halt = tegra_xudc_ep_set_halt,
>>>> +};
>>>
>>> What makes EP0 special?
>>>
>>
>> EP0 is always enabled and hence any calls to enable/disable are not handled
>> hence separate ops for EP0 to reject request if any
>
> -EINVAL sounds suboptimal as an error message in this case. Perhaps
> something like -EBUSY would be more appropriate?
>
will move to EBUSY
> [...]
>>>> + return err;
>>>> + }
>>>> +
>>>> + err = tegra_xudc_clk_init(xudc);
>>>> + if (err < 0) {
>>>> + dev_err(xudc->dev, "failed to init clocks %d\n", err);
>>>> + return err;
>>>> + }
>>>> +
>>>> + xudc->supplies = devm_kcalloc(&pdev->dev, xudc->soc->num_supplies,
>>>> + sizeof(*xudc->supplies), GFP_KERNEL);
>>>> + if (!xudc->supplies)
>>>> + return -ENOMEM;
>>>> + for (i = 0; i < xudc->soc->num_supplies; i++)
>>>> + xudc->supplies[i].supply = xudc->soc->supply_names[i];
>>>> + err = devm_regulator_bulk_get(&pdev->dev, xudc->soc->num_supplies,
>>>> + xudc->supplies);
>>>> + if (err) {
>>>> + dev_err(xudc->dev, "failed to request regulators %d\n", err);
>>>> + return err;
>>>> + }
>>>> +
>>>> + xudc->padctl = tegra_xusb_padctl_get(&pdev->dev);
>>>> + if (IS_ERR(xudc->padctl))
>>>> + return PTR_ERR(xudc->padctl);
>>>
>>> It seems like we only need this for the VBUS override functionality. If
>>> we can get rid of that by incorporating it into phy API functions, we
>>> could remove the need to get a reference to it here.
>>>
>>
>> As mentioned earlier, we need this for port operations
>
> Okay, let's keep it, then.
>
> [...]
>>>> +#ifdef CONFIG_PM
>>>> +static int tegra_xudc_runtime_suspend(struct device *dev)
>>>> +{
>>>> + struct tegra_xudc *xudc = dev_get_drvdata(dev);
>>>> + unsigned long flags;
>>>> +
>>>> + spin_lock_irqsave(&xudc->lock, flags);
>>>> + if (WARN_ON(xudc->device_mode)) {
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>> + return -EBUSY;
>>>> + }
>>>> + spin_unlock_irqrestore(&xudc->lock, flags);
>>>
>>> This seems overly restrictive. Why reject suspend mode when we're in
>>> device mode?
>>>
>>> Also, it seems to me like there really aren't any differences between
>>> system sleep and runtime PM, so why not just leave away the system sleep
>>> functions and just let runtime PM deal with everything?
>>>
>> During system sleep we force off the device mode and not in runtime suspend
>> and viceversa during resume, in runtime, its more of powergaring
>
> Come to think of it, why would we even attempt to runtime suspend if
> we're in device mode? Can't we just make sure that we call
> pm_runtime_get() and pm_runtime_put() in the right places so that this
> doesn't happen? Or is this again the race condition that can happen if
> we quickly plug/unplug?
>
pm run time is refcount based and if device_mode is on, we cannot enter runtime suspend,
since device mode on resumes interface. Done full code through one more time, with mutex lock
done for device_mode and places where runtimes are called, dont see a case where this condition
occurs, will remove this WARN_ON
-Nagarjuna
> Thierry
>
