Re: [PATCH] usb: ohci-sa1111: convert shutdown method to native device_driver

[Russell King - ARM Linux]

On Tue, Sep 26, 2017 at 11:35:32AM -0400, Alan Stern wrote:
> On Tue, 26 Sep 2017, Russell King - ARM Linux wrote:
> 
> > On Tue, Sep 26, 2017 at 10:35:23AM -0400, Alan Stern wrote:
> > > On Tue, 26 Sep 2017, Russell King wrote:
> > > > Convert the shutdown method to use the device_driver shutdown function
> > > > pointer rather than a private bus-type shutdown.  This is the only user
> > > > for SA bus types, so having the support code in the bus doesn't
> > > > make any sense.
> 
> > > I have no objection to this patch.  But it leads me to wonder why you 
> > > don't get rid of the SA bus type entirely, rather than keeping it 
> > > just for the sake of one driver?
> > 
> > I think you misunderstood the commit message.  This is the only user of
> > the shutdown method for this bus type.  This is not the only user of
> > this bus type - there are other drivers that use this bus type.
> 
> I see -- just a slight ambiguity in the description.  That's fine.
> 
> For all three of the ohci-sa patches:
> 
> Acked-by: Alan Stern <st...@rowland.harvard.edu>

Thanks, I've improved the commit message to clear up that misunderstanding.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: ohci-sa1111: convert shutdown method to native device_driver

[Russell King - ARM Linux]

On Tue, Sep 26, 2017 at 10:35:23AM -0400, Alan Stern wrote:
> On Tue, 26 Sep 2017, Russell King wrote:
> > Convert the shutdown method to use the device_driver shutdown function
> > pointer rather than a private bus-type shutdown.  This is the only user
> > for SA bus types, so having the support code in the bus doesn't
> > make any sense.
> > 
> > Signed-off-by: Russell King 
> > ---
> >  drivers/usb/host/ohci-sa.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ohci-sa.c b/drivers/usb/host/ohci-sa.c
> > index 9aa4fe1800b9..82842918cb0c 100644
> > --- a/drivers/usb/host/ohci-sa.c
> > +++ b/drivers/usb/host/ohci-sa.c
> > @@ -247,8 +247,9 @@ static int ohci_hcd_sa_remove(struct sa_dev 
> > *dev)
> > return 0;
> >  }
> >  
> > -static void ohci_hcd_sa_shutdown(struct sa_dev *dev)
> > +static void ohci_hcd_sa_shutdown(struct device *_dev)
> >  {
> > +   struct sa_dev *dev = to_sa_device(_dev);
> > struct usb_hcd *hcd = sa_get_drvdata(dev);
> >  
> > if (test_bit(HCD_FLAG_HW_ACCESSIBLE, >flags)) {
> > @@ -261,9 +262,9 @@ static struct sa_driver ohci_hcd_sa_driver = {
> > .drv = {
> > .name   = "sa-ohci",
> > .owner  = THIS_MODULE,
> > +   .shutdown = ohci_hcd_sa_shutdown,
> > },
> > .devid  = SA_DEVID_USB,
> > .probe  = ohci_hcd_sa_probe,
> > .remove = ohci_hcd_sa_remove,
> > -   .shutdown   = ohci_hcd_sa_shutdown,
> >  };
> 
> I have no objection to this patch.  But it leads me to wonder why you 
> don't get rid of the SA bus type entirely, rather than keeping it 
> just for the sake of one driver?

I think you misunderstood the commit message.  This is the only user of
the shutdown method for this bus type.  This is not the only user of
this bus type - there are other drivers that use this bus type.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 0/7] Clean up PHY MMD accessors

[Russell King - ARM Linux]

This series cleans up phylib's MMD accessors, so that we have a common
way of accessing the Clause 45 register set.

The current situation is far from ideal - we have phy_(read|write)_mmd()
which accesses Clause 45 registers over Clause 45 accesses, and we have
phy_(read|write)_mmd_indirect(), which accesses Clause 45 registers via
Clause 22 register 13/14.

Generic code uses the indirect methods to access standard Clause 45
features, and when we come to add Clause 45 PHY support to phylib, we
would need to make these conditional upon the PHY type, or duplicate
these functions.

An alternative solution is to merge these accessors together, and select
the appropriate access method depending upon the 802.3 clause that the
PHY conforms with.  The result is that we have a single set of
phy_(read|write)_mmd() accessors.

For cases which require special handling, we still allow PHY drivers to
override all MMD accesses - except rather than just overriding the
indirect accesses.  This keeps existing overrides working.

Combining the two also has another beneficial side effect - we get rid
of similar functions that take arguments in different orders.  The
old direct accessors took the phy structure, devad and register number,
whereas the indirect accessors took the phy structure, register number
and devad in that order.  Care must be taken when updating future
drivers that the argument order is correct, and the function name is
not merely replaced.

This patch set is against net-next.

 drivers/net/phy/Makefile  |   2 +-
 drivers/net/phy/bcm-phy-lib.c |  12 ++---
 drivers/net/phy/dp83867.c |  25 +-
 drivers/net/phy/intel-xway.c  |  26 +-
 drivers/net/phy/micrel.c  |  13 +++--
 drivers/net/phy/microchip.c   |   5 +-
 drivers/net/phy/phy-core.c| 101 ++
 drivers/net/phy/phy.c | 110 --
 drivers/net/phy/phy_device.c  |   4 +-
 drivers/net/usb/lan78xx.c |  10 ++--
 include/linux/phy.h   |  80 +-
 11 files changed, 178 insertions(+), 210 deletions(-)

RFC->v1:
- minor update patch 2 to resolve a build error that the 0-day builder
  discovered.
- reviewed-by / acked-bys added

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/7] phylib MMD accessor cleanups

[Russell King - ARM Linux]

On Sun, Mar 19, 2017 at 03:30:38PM -0700, Florian Fainelli wrote:
> Le 03/19/17 à 03:59, Russell King - ARM Linux a écrit :
> > This series of patches does exactly that - we merge the functionality
> > of the indirect accesses into the clause 45 accessors, and use these
> > exclusively to access MMD registers.  Internally, the new clause
> > independent MMD accessors indirect via the PHY drivers read_mmd/write_mmd
> > methods if present, otherwise fall back to using clause 45 accesses if
> > the PHY is a clause 45 phy, or clause 22 indirect accesses if the PHY
> > is clause 22.
> 
> LGTM:
> 
> Reviewed-by: Florian Fainelli <f.faine...@gmail.com>

Thanks.  When I posted this last time around (19th Jan) I mentioned
about marking the old _indirect() accessors with __deprecated - is
that still something we want to do?

I haven't tested this against net-next yet, so I don't know if there
are any new users of the indirect accessors - going down the deprecated
route would avoid breakage, but means having to submit a patch later to
actually remove them.

How would people want this handled?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 0/7] phylib MMD accessor cleanups

[Russell King - ARM Linux]

Hi,

This series cleans up the phylib MMD accessors.  We have two accessors
at present, phy_(read|write)_mmd() and phy_(read|write)_mmd_indirect()

The _indirect methods access the MMD registers via a clause 22 phy,
whereas the non-_indirect methods acess via clause 45 accesses.

Current PHY-independent parts of phylib (such as EEE) access the MMD
registers via the _indirect methods, which is no good if you have a
clause 45 PHY that doesn't respond to clause 22 accesses.

In order to make these features available, we need to rework these
accessors such that they can access the MMD registers using a method
dependent on the clause that the PHY conforms with.

This series of patches does exactly that - we merge the functionality
of the indirect accesses into the clause 45 accessors, and use these
exclusively to access MMD registers.  Internally, the new clause
independent MMD accessors indirect via the PHY drivers read_mmd/write_mmd
methods if present, otherwise fall back to using clause 45 accesses if
the PHY is a clause 45 phy, or clause 22 indirect accesses if the PHY
is clause 22.

Note: confusingly, phy_read_mmd_indirect() vs phy_read_mmd() switches
the order of prtad and devad, which means that converting between the
two is not a simple matter of just replacing the function name.  Same
applies for the write methods.

 drivers/net/phy/Makefile  |   2 +-
 drivers/net/phy/bcm-phy-lib.c |  12 ++---
 drivers/net/phy/dp83867.c |  25 +-
 drivers/net/phy/intel-xway.c  |  26 +-
 drivers/net/phy/micrel.c  |  13 +++--
 drivers/net/phy/microchip.c   |   5 +-
 drivers/net/phy/phy-core.c| 101 ++
 drivers/net/phy/phy.c | 110 --
 drivers/net/phy/phy_device.c  |   4 +-
 drivers/net/usb/lan78xx.c |  10 ++--
 include/linux/phy.h   |  80 +-
 11 files changed, 178 insertions(+), 210 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.10-rc3 00/13] net: dsa: remove unnecessary phy.h include

[Russell King - ARM Linux]

Including phy.h and phy_fixed.h into net/dsa.h causes phy*.h to be an
unnecessary dependency for quite a large amount of the kernel.  There's
very little which actually requires definitions from phy.h in net/dsa.h
- the include itself only wants the declaration of a couple of
structures and IFNAMSIZ.

Add linux/if.h for IFNAMSIZ, declarations for the structures, phy.h to
mv88e6xxx.h as it needs it for phy_interface_t, and remove both phy.h
and phy_fixed.h from net/dsa.h.

This patch reduces from around 800 files rebuilt to around 40 - even
with ccache, the time difference is noticable.

In order to make this change, several drivers need to be updated to
include necessary headers that they were picking up through this
include.  This has resulted in a much larger patch series.

I'm assuming the 0-day builder has had 24 hours with this series, and
hasn't reported any further issues with it - the last issue was two
weeks ago (before I became ill) which I fixed over the last weekend.

I'm hoping this doesn't conflict with what's already in net-next...

 arch/mips/cavium-octeon/octeon-platform.c | 4 
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 1 +
 drivers/net/ethernet/broadcom/bgmac.c | 2 ++
 drivers/net/ethernet/cadence/macb.h   | 2 ++
 drivers/net/ethernet/cavium/liquidio/lio_main.c   | 1 +
 drivers/net/ethernet/cavium/liquidio/lio_vf_main.c| 1 +
 drivers/net/ethernet/cavium/liquidio/octeon_console.c | 1 +
 drivers/net/ethernet/freescale/fman/fman_memac.c  | 1 +
 drivers/net/ethernet/marvell/mvneta.c | 1 +
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c   | 1 +
 drivers/net/usb/lan78xx.c | 1 +
 drivers/net/wireless/ath/ath5k/ahb.c  | 2 +-
 drivers/target/iscsi/iscsi_target_login.c | 1 +
 include/net/dsa.h | 6 --
 net/core/netprio_cgroup.c | 1 +
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c| 1 +
 16 files changed, 20 insertions(+), 7 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/7] Clean up PHY MMD accessors

[Russell King - ARM Linux]

On Thu, Jan 19, 2017 at 11:42:47AM -0800, Florian Fainelli wrote:
> On 01/13/2017 07:20 AM, Russell King - ARM Linux wrote:
> > This series cleans up phylib's MMD accessors, so that we have a common
> > way of accessing the Clause 45 register set.
> > 
> > The current situation is far from ideal - we have phy_(read|write)_mmd()
> > which accesses Clause 45 registers over Clause 45 accesses, and we have
> > phy_(read|write)_mmd_indirect(), which accesses Clause 45 registers via
> > Clause 22 register 13/14.
> > 
> > Generic code uses the indirect methods to access standard Clause 45
> > features, and when we come to add Clause 45 PHY support to phylib, we
> > would need to make these conditional upon the PHY type, or duplicate
> > these functions.
> > 
> > An alternative solution is to merge these accessors together, and select
> > the appropriate access method depending upon the 802.3 clause that the
> > PHY conforms with.  The result is that we have a single set of
> > phy_(read|write)_mmd() accessors.
> > 
> > For cases which require special handling, we still allow PHY drivers to
> > override all MMD accesses - except rather than just overriding the
> > indirect accesses.  This keeps existing overrides working.
> > 
> > Combining the two also has another beneficial side effect - we get rid
> > of similar functions that take arguments in different orders.  The
> > old direct accessors took the phy structure, devad and register number,
> > whereas the indirect accessors took the phy structure, register number
> > and devad in that order.  Care must be taken when updating future
> > drivers that the argument order is correct, and the function name is
> > not merely replaced.
> 
> I really like that series, this is much much cleaner, do you mind
> resubmitting this without a RFC tag so David can apply it?

Hi Florian,

Thanks.

Will do in the next couple of days - I'd prefer to make one change
to the series before submitting it for real: provide compatibility
functions marked as deprecated so this doesn't create a flag day.
We can remove the deprecated _indirect functions later when we know
there's no new users.

I'd like to get the phy.h untanglement stuff into David's tree first.
I'm almost there, my allmodconfig from last night almost built fine
but for something I missed in bgmac.c, and 0-day found a MIPS file
affected by the change.  New code pushed out for 0-day and I'm
expecting tonights allmodconfig to be clear.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 0/7] Clean up PHY MMD accessors

[Russell King - ARM Linux]

This series cleans up phylib's MMD accessors, so that we have a common
way of accessing the Clause 45 register set.

The current situation is far from ideal - we have phy_(read|write)_mmd()
which accesses Clause 45 registers over Clause 45 accesses, and we have
phy_(read|write)_mmd_indirect(), which accesses Clause 45 registers via
Clause 22 register 13/14.

Generic code uses the indirect methods to access standard Clause 45
features, and when we come to add Clause 45 PHY support to phylib, we
would need to make these conditional upon the PHY type, or duplicate
these functions.

An alternative solution is to merge these accessors together, and select
the appropriate access method depending upon the 802.3 clause that the
PHY conforms with.  The result is that we have a single set of
phy_(read|write)_mmd() accessors.

For cases which require special handling, we still allow PHY drivers to
override all MMD accesses - except rather than just overriding the
indirect accesses.  This keeps existing overrides working.

Combining the two also has another beneficial side effect - we get rid
of similar functions that take arguments in different orders.  The
old direct accessors took the phy structure, devad and register number,
whereas the indirect accessors took the phy structure, register number
and devad in that order.  Care must be taken when updating future
drivers that the argument order is correct, and the function name is
not merely replaced.

This patch set is against 4.10-rc3 at present.

 drivers/net/phy/Makefile  |   3 +-
 drivers/net/phy/bcm-phy-lib.c |  12 ++---
 drivers/net/phy/dp83867.c |  18 +++
 drivers/net/phy/intel-xway.c  |  26 +-
 drivers/net/phy/micrel.c  |  13 +++--
 drivers/net/phy/microchip.c   |   5 +-
 drivers/net/phy/phy-core.c| 101 ++
 drivers/net/phy/phy.c | 110 --
 drivers/net/phy/phy_device.c  |   4 +-
 drivers/net/usb/lan78xx.c |  10 ++--
 include/linux/phy.h   |  56 +
 11 files changed, 176 insertions(+), 182 deletions(-)
 create mode 100644 drivers/net/phy/phy-core.c

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] ARM: dma: fix dma_max_pfn()

[Russell King - ARM Linux]

On Wed, Sep 28, 2016 at 10:53:49AM +0300, Roger Quadros wrote:
> Hi,
> 
> On 12/09/16 14:38, Roger Quadros wrote:
> > Hi Santosh & Russell,
> > 
> > On 19/08/16 19:38, Santosh Shilimkar wrote:
> >>
> >> On 8/19/2016 12:30 AM, Roger Quadros wrote:
> >>> Hi Santosh,
> >>>
> >>
> > So I'm 99.9% convinced that the proposed change is correct.
> >
>  I will got with that then :-) and take my objection back. Just
>  saying that if there other breakages which I can't recollect now,
>  those drivers needs to be patched as well.
> 
> >>> I was able to boot the Keystone2 Edison EVM over NFS with the $subject 
> >>> patch.
> >>> Boot log is below. Do you see anything suspicious?
> >>>
> >> Logs looks ok to me. Probably do some tests where DMA and bounce buffers 
> >> etc gets tested. Running it through your internal regression
> >> suit will be good idea as well if thats possible.
> >>
> > 
> > This has been running in our internal test suite for a week on various TI
> > platforms. There haven't been any surprises.
> > 
> > Is it a good idea to at least put this in -next for a wider test audience?
> 
> Gentle reminder.

Please put it in the patch system, thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Russell King - ARM Linux]

On Wed, Sep 07, 2016 at 05:29:01PM +0800, Peter Chen wrote:
> On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote:
> > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
> > > 
> > > The pre-condition of DT function at USB HCD core works is the host
> > > controller device has of_node, since it is the root node for USB tree
> > > described at DT. If the host controller device is not at DT, it needs
> > > to try to get its of_node, the chipidea driver gets it through its
> > > parent node [1]
> > 
> > > 
> > > [1] https://lkml.org/lkml/2016/8/8/119
> > > 
> > 
> > Ah, this is what I was referring to in the other mail.
> > 
> > However, the way you set the of_node might be dangerous too:
> > We should generally not have two platform_device structures with
> > the same of_node pointer, most importantly it may cause the
> > child device to be bound to the same driver as the parent
> > device since the probing is done by compatible string.
> > 
> > As you tested it successfully, it must work at the moment on your
> > machine, but it could easily break depending on deferred probing
> > or module load order.
> > 
> 
> Currently, I work around above problems by setting core device of_node
> as NULL at both probe error path and platform driver .remove routine.
> 
> I admit it is not a good way, but if we only have of_node at device's
> life periods after probe, it seems ok currently. It is hard to create
> of_node dynamically when create device, and keep some contents
> of parent's of_node, and some are not.

How about turning dwc3 into a library which can be used by a range of
platform devices?  Wouldn't that solve all the current problems, and
completely avoid the need to copy resources from one platform device
to another?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Russell King - ARM Linux]

On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote:
> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote:
> > 
> > Hi Felipe and Arnd,
> > 
> > It has been a while since the last response to this discussion, but we
> > haven't reached an agreement yet!  Can we get to a conclusion on if it
> > is valid to create child platform device for abstraction purpose?  If
> > yes, can this child device do DMA by itself?
> 
> I'd say it's no problem for a driver to create child devices in order
> to represent different aspects of a device, but you should not rely on
> those devices working when used with the dma-mapping interfaces.

That's absolutely right.  Consider the USB model - only the USB host
controller can perform DMA, not the USB devices themselves.  All DMA
mappings need to be mapped using the USB host controller device struct
not the USB device struct.

The same _should_ be true everywhere else: the struct device representing
the device performing DMA must be the one used to map the transfer.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: ohci-sa1111: remove machine_has_neponset()

[Russell King - ARM Linux]

On Fri, Aug 26, 2016 at 02:20:50PM -0400, Alan Stern wrote:
> On Fri, 26 Aug 2016, Russell King - ARM Linux wrote:
> 
> > On Fri, Aug 26, 2016 at 01:18:25PM -0400, Alan Stern wrote:
> > > On Fri, 26 Aug 2016, Russell King wrote:
> > > 
> > > > The neponset is a daughter board for the Assabet platform, which has a
> > > > SA chip on it.  If we're initialising the SA OHCI, and we're
> > > > part of a neponset, the host platform must be an Assabet.
> > > > 
> > > > This allows us to eliminate machine_has_neponset() from this driver,
> > > > replacing it instead with machine_is_assabet(), and killing the
> > > > mach/assabet.h include.
> > > 
> > > Silly question: What happens when there's an SA OHCI controller on
> > > an Assabet platform, but contained in something other than a neponset
> > > daughterboard?
> > 
> > It's possible that there is such a thing, but unlikely - and we have
> > no code in mainline to support such a configuration.
> 
> In that case you can add:
> 
> Acked-by: Alan Stern <st...@rowland.harvard.edu>
> 
> to this and the 2/2 patch.

Thanks!

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: ohci-sa1111: remove machine_has_neponset()

[Russell King - ARM Linux]

On Fri, Aug 26, 2016 at 01:18:25PM -0400, Alan Stern wrote:
> On Fri, 26 Aug 2016, Russell King wrote:
> 
> > The neponset is a daughter board for the Assabet platform, which has a
> > SA chip on it.  If we're initialising the SA OHCI, and we're
> > part of a neponset, the host platform must be an Assabet.
> > 
> > This allows us to eliminate machine_has_neponset() from this driver,
> > replacing it instead with machine_is_assabet(), and killing the
> > mach/assabet.h include.
> 
> Silly question: What happens when there's an SA OHCI controller on
> an Assabet platform, but contained in something other than a neponset
> daughterboard?

It's possible that there is such a thing, but unlikely - and we have
no code in mainline to support such a configuration.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: ohci-omap - avoid including mach/irqs.h

[Russell King - ARM Linux]

On Fri, Aug 26, 2016 at 01:20:53PM -0400, Alan Stern wrote:
> On Fri, 26 Aug 2016, Russell King - ARM Linux wrote:
> 
> > On Mon, Aug 22, 2016 at 11:28:21AM -0400, Alan Stern wrote:
> > > On Fri, 19 Aug 2016, Russell King wrote:
> > > 
> > > > ohci-omap doesn't need to include mach/irqs.h - nothing within this
> > > > driver needs anything from this header file.  Remove this include.
> > > > 
> > > > Signed-off-by: Russell King <rmk+ker...@armlinux.org.uk>
> > > > ---
> > > >  drivers/usb/host/ohci-omap.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
> > > > index de7c68602a7e..495c1454b9e8 100644
> > > > --- a/drivers/usb/host/ohci-omap.c
> > > > +++ b/drivers/usb/host/ohci-omap.c
> > > > @@ -36,7 +36,6 @@
> > > >  #include 
> > > >  
> > > >  #include 
> > > > -#include 
> > > >  #include 
> > > 
> > > Acked-by: Alan Stern <st...@rowland.harvard.edu>
> > 
> > Thanks.  Will Greg be picking this up?
> 
> Yes, he should.  Unless you want it to go via a different tree...

I'm fine with Greg picking this (and the other two) up as none
of my (increasing) patch stack depends on these patches.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: ohci-omap - avoid including mach/irqs.h

[Russell King - ARM Linux]

On Mon, Aug 22, 2016 at 11:28:21AM -0400, Alan Stern wrote:
> On Fri, 19 Aug 2016, Russell King wrote:
> 
> > ohci-omap doesn't need to include mach/irqs.h - nothing within this
> > driver needs anything from this header file.  Remove this include.
> > 
> > Signed-off-by: Russell King 
> > ---
> >  drivers/usb/host/ohci-omap.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
> > index de7c68602a7e..495c1454b9e8 100644
> > --- a/drivers/usb/host/ohci-omap.c
> > +++ b/drivers/usb/host/ohci-omap.c
> > @@ -36,7 +36,6 @@
> >  #include 
> >  
> >  #include 
> > -#include 
> >  #include 
> 
> Acked-by: Alan Stern 

Thanks.  Will Greg be picking this up?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] ARM: dma: fix dma_max_pfn()

[Russell King - ARM Linux]

On Thu, Aug 18, 2016 at 09:55:55AM -0700, Santosh Shilimkar wrote:
> Hi Russell,
> 
> On 8/18/2016 7:24 AM, Russell King - ARM Linux wrote:
> >On Wed, Aug 17, 2016 at 03:05:17PM +0300, Roger Quadros wrote:
> >>Since commit 6ce0d2001692 ("ARM: dma: Use dma_pfn_offset for dma address 
> >>translation"),
> >>dma_to_pfn() already returns the PFN with the physical memory start offset
> >>so we don't need to add it again.
> >>
> >>This fixes USB mass storage lock-up problem on systems that can't do DMA
> >>over the entire physical memory range (e.g.) Keystone 2 systems with 4GB RAM
> >>can only do DMA over the first 2GB. [K2E-EVM].
> >>
> >>What happens there is that without this patch SCSI layer sets a wrong
> >>bounce buffer limit in scsi_calculate_bounce_limit() for the USB mass
> >>storage device. dma_max_pfn() evaluates to 0x8f and bounce_limit
> >>is set to 0x8f000 whereas maximum DMA'ble physical memory on Keystone 2
> >>is 0x87fff. This results in non DMA'ble pages being given to the
> >>USB controller and hence the lock-up.
> >>
> >>NOTE: in the above case, USB-SCSI-device's dma_pfn_offset was showing as 0.
> >>This should have really been 0x78 as on K2e, LOWMEM_START is 0x8000
> >>and HIGHMEM_START is 0x8. DMA zone is 2GB so dma_max_pfn should be
> >>0x87. The incorrect dma_pfn_offset for the USB storage device is because
> >>USB devices are not correctly inheriting the dma_pfn_offset from the
> >>USB host controller. This will be fixed by a separate patch.
> >
> >I'd like to hear from Santosh, as the author of the original change.
> >The original commit doesn't mention which platform it was intended for
> >or what the problem was, which would've been helpful.
> >
> From what I recollect, we did these changes to make the max pfn behave
> same on ARM arch as other archs. This patch was evolved as part of
> fixing the max*pfn assumption.

To me, the proposed patch _looks_ correct, because...

> >>diff --git a/arch/arm/include/asm/dma-mapping.h 
> >>b/arch/arm/include/asm/dma-mapping.h
> >>index d009f79..bf02dbd 100644
> >>--- a/arch/arm/include/asm/dma-mapping.h
> >>+++ b/arch/arm/include/asm/dma-mapping.h
> >>@@ -111,7 +111,7 @@ static inline dma_addr_t virt_to_dma(struct device 
> >>*dev, void *addr)
> >> /* The ARM override for dma_max_pfn() */
> >> static inline unsigned long dma_max_pfn(struct device *dev)
> >> {
> >>-   return PHYS_PFN_OFFSET + dma_to_pfn(dev, *dev->dma_mask);
> >>+   return dma_to_pfn(dev, *dev->dma_mask);
> >> }
> >> #define dma_max_pfn(dev) dma_max_pfn(dev)
> By doing this change I hope we don't break other drivers on Keystone so
> am not sure about the change.

dma_to_pfn() returns the page frame number referenced from physical
address zero - the default implementation of dma_to_pfn() is
bus_to_pfn(), which is __phys_to_pfn(x), which is just x >> PAGE_SHIFT.
The other thing about dma_to_pfn() is that it should return a
zero-referenced PFN number, where PFN 0 = physical address 0.

If there is some offset for keystone2, that should be taken care of
via "dev->dma_pfn_offset", and not offsetting the return value from
dma_to_pfn().

So I'm 99.9% convinced that the proposed change is correct.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] ARM: dma: fix dma_max_pfn()

[Russell King - ARM Linux]

On Wed, Aug 17, 2016 at 03:05:17PM +0300, Roger Quadros wrote:
> Since commit 6ce0d2001692 ("ARM: dma: Use dma_pfn_offset for dma address 
> translation"),
> dma_to_pfn() already returns the PFN with the physical memory start offset
> so we don't need to add it again.
> 
> This fixes USB mass storage lock-up problem on systems that can't do DMA
> over the entire physical memory range (e.g.) Keystone 2 systems with 4GB RAM
> can only do DMA over the first 2GB. [K2E-EVM].
> 
> What happens there is that without this patch SCSI layer sets a wrong
> bounce buffer limit in scsi_calculate_bounce_limit() for the USB mass
> storage device. dma_max_pfn() evaluates to 0x8f and bounce_limit
> is set to 0x8f000 whereas maximum DMA'ble physical memory on Keystone 2
> is 0x87fff. This results in non DMA'ble pages being given to the
> USB controller and hence the lock-up.
> 
> NOTE: in the above case, USB-SCSI-device's dma_pfn_offset was showing as 0.
> This should have really been 0x78 as on K2e, LOWMEM_START is 0x8000
> and HIGHMEM_START is 0x8. DMA zone is 2GB so dma_max_pfn should be
> 0x87. The incorrect dma_pfn_offset for the USB storage device is because
> USB devices are not correctly inheriting the dma_pfn_offset from the
> USB host controller. This will be fixed by a separate patch.

I'd like to hear from Santosh, as the author of the original change.
The original commit doesn't mention which platform it was intended for
or what the problem was, which would've been helpful.

> 
> Fixes: 6ce0d2001692 ("ARM: dma: Use dma_pfn_offset for dma address 
> translation")
> Cc: sta...@vger.kernel.org
> Cc: Greg Kroah-Hartman 
> Cc: Russell King 
> Cc: Arnd Bergmann 
> Cc: Olof Johansson 
> Cc: Catalin Marinas 
> Cc: Linus Walleij 
> Reported-by: Grygorii Strashko 
> Signed-off-by: Roger Quadros 
> ---
>  arch/arm/include/asm/dma-mapping.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/dma-mapping.h 
> b/arch/arm/include/asm/dma-mapping.h
> index d009f79..bf02dbd 100644
> --- a/arch/arm/include/asm/dma-mapping.h
> +++ b/arch/arm/include/asm/dma-mapping.h
> @@ -111,7 +111,7 @@ static inline dma_addr_t virt_to_dma(struct device *dev, 
> void *addr)
>  /* The ARM override for dma_max_pfn() */
>  static inline unsigned long dma_max_pfn(struct device *dev)
>  {
> - return PHYS_PFN_OFFSET + dma_to_pfn(dev, *dev->dma_mask);
> + return dma_to_pfn(dev, *dev->dma_mask);
>  }
>  #define dma_max_pfn(dev) dma_max_pfn(dev)
>  
> -- 
> 2.7.4
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node equal's glue layer device of_node

[Russell King - ARM Linux]

On Thu, Jul 21, 2016 at 05:20:12PM +0800, Peter Chen wrote:
> On Thu, Jul 21, 2016 at 10:14:38AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jul 20, 2016 at 05:40:28PM +0800, Peter Chen wrote:
> > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > index 69426e6..0d05812 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -914,6 +914,16 @@ static int ci_hdrc_probe(struct platform_device 
> > > *pdev)
> > >   if (!ci)
> > >   return -ENOMEM;
> > >  
> > > + /*
> > > +  * At device tree, we have no device node for chipidea core,
> > > +  * the glue layer's node is the parent node for host and udc
> > > +  * device. But in related driver, the parent device is chipidea
> > > +  * core. So, in order to let the common driver get parent's node,
> > > +  * we let the core's device node equals glue layer's node.
> > > +  */
> > > + if (dev->parent && dev->parent->of_node)
> > > + dev->of_node = dev->parent->of_node;
> > 
> > This is a dangerous thing to do.  You're changing the dev->of_node of
> > _this_ device, which means that _this_ driver will no longer match
> > the device if you remove and reinsert the driver module, or unbind
> > and try to re-bind the device to this driver.
> > 
> 
> Thanks for commenting it.
> 
> I have tested load/unload, it does not show any problems.
> 
> The chipidea core device is created by code at runtime, not by device node.
> And we have NO device node for this chipidea core device at dts.

Okay, so we still probably have the bind/unbind problem, where "dev"
can be matched by the driver which claimed "dev->parent".  Remember,
in an OF environment, driver matching is done by the compatible
property, which is accessed via dev->of_node.

Therefore, I would suggest that you NULL dev->of_node in the error
cleanup paths and in the remove function, so you don't have an
unbound device with a duplicated (but inappropriate) dev->of_node
pointer.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node equal's glue layer device of_node

[Russell King - ARM Linux]

On Wed, Jul 20, 2016 at 05:40:28PM +0800, Peter Chen wrote:
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 69426e6..0d05812 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -914,6 +914,16 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>   if (!ci)
>   return -ENOMEM;
>  
> + /*
> +  * At device tree, we have no device node for chipidea core,
> +  * the glue layer's node is the parent node for host and udc
> +  * device. But in related driver, the parent device is chipidea
> +  * core. So, in order to let the common driver get parent's node,
> +  * we let the core's device node equals glue layer's node.
> +  */
> + if (dev->parent && dev->parent->of_node)
> + dev->of_node = dev->parent->of_node;

This is a dangerous thing to do.  You're changing the dev->of_node of
_this_ device, which means that _this_ driver will no longer match
the device if you remove and reinsert the driver module, or unbind
and try to re-bind the device to this driver.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Russell King - ARM Linux]

On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Arnd Bergmann  writes:
> >pointer and pass that in platform_data. This is really easy, it's
> 
> Sorry but passing a struct device pointer in platform_data is
> ridiculous. Not to mention that, as I said before, we can't assume which
> device to pass to xhci_plat in the first place. It might be dwc->dev and
> it might be dwc->dev->parent.

+1.  Passing an unref-counted struct device through platform data is
totally mad, Arnd you're off your rocker if you think that's a good
idea.  What's more is that there's no way to properly refcount the
thing.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: chipidea: Configure DMA properties and ops from DT

[Russell King - ARM Linux]

On Fri, Mar 18, 2016 at 09:54:14AM +0800, Peter Chen wrote:
> Although I don't know what kinds of bugs it may have, it may be
> met before, otherwise, why most of platform drivers need to call
> dma_set_coherent_mask or dma_coerce_mask_and_coherent explicitly

See Documentation/DMA-API.txt, specifically the section starting

  Part Ic - DMA addressing limitations
  

and also Documentation/DMA-API-HOWTO.txt, the section on

  DMA addressing limitations

which provides further information.

Drivers using DMA should be using dma_set_mask_and_coherent() _or_
one of dma_set_mask() and dma_set_coherent_mask() depending on which
types of DMA they wish to perform.  Drivers should not use
dma_coerce_mask_and_coherent() except in exceptional circumstances:
that function is more a marker that they or some bus/platform code
is doing something wrong.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Thu, Oct 22, 2015 at 07:44:05AM -0700, Greg Kroah-Hartman wrote:
> 
> 
> On Thu, Oct 22, 2015 at 11:05:11AM +0200, Tomeu Vizoso wrote:
> > Given that downstreams are already carrying as many hacks as they
> > could think of to speed total boot up, I think this is effectively
> > telling them to go away.
> 
> No I'm not, I'm asking for real data, not hand-wavy-this-is-going-to
> solve-the-random-issue-i'm-having type patch by putting random calls in
> semi-random subsystems all over the kernel.

+1000, fully agree.

There's too much verbal diarrhoea going on in this thread and no facts.
I've been waiting for real data too, and there's not one shred of it, or
even a hint that it might appear.  So, the conclusion I'm coming to is
that there isn't any data to back up the claims made in this thread.

If it was such a problem, then in the _eight_ days that this has been
discussed so far, _someone_ would have sent some data showing the
problem.  I think the fact is, there is no data.

Someone prove me wrong.  Someone post the verifiable data showing that
there is a problem to be solved here.

Someone show what the specific failure cases are that are hampering
vendors moving forwards.  Someone show the long boot times by way of
kernel message log.  Someone show some evidence of the problems that
have been alluded to.

If no one can show some evidence, there isn't a problem here. :)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alternative approach to solve the deferred probe

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 09:13:48PM +0300, Grygorii Strashko wrote:
> But I worry a bit (and that my main point) about these few additional
> rounds of deferred device probing which I have right now and which allows
> some of drivers to finish, finally, their probes successfully.
> With proposed change I'll get more messages in boot log, but some of
> them will belong to drivers which have been probed successfully and so,
> they will be not really useful.

Then you haven't properly understood my proposal.

I want to get rid of all the "X deferred its probing" messages up until
the point that we set the "please report deferred probes" flag.

That _should_ mean that all the deferred probing that goes on becomes
_totally_ silent and becomes hidden (unless you really want to see it,
in which case we can make a debug option which turns it on) up until
we're at the point where we want to enter userspace.

At that point, we then report into the kernel log which devices are
still deferring and, via appropriately placed dev_warn_deferred(),
the reasons why the devices are being deferred.

So, gone will be all the messages earlier in the log about device X
not having a GPIO/clock/whatever because the device providing the
GPIO/clock/whatever hasn't been probed.

If everything is satisfied by the time we run this last round (again,
I'm not using a three line sentence to describe exactly what I mean,
I'm sure you know by now... oops, I just did) then the kernel will
report nothing about any deferrals.  That's _got_ to be an improvement.

> 
> As result, I think, the most important thing is to identify (or create)
> some point during kernel boot when it will be possible to say that all
> built-in drivers (at least) finish their probes 100% (done or defer).
> 
> Might be do_initcalls() can be updated (smth like this):
> static void __init do_initcalls(void)
> {
>   int level;
> 
>   for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
>   do_initcall_level(level);
> 
> + wait_for_device_probe();
> + /* Now one final round, reporting any devices that remain deferred */
> + driver_deferred_probe_report = true;
> + driver_deferred_probe_trigger();
> + wait_for_device_probe();
> }
> 
> Also, in my opinion, it will be useful if this debugging feature will be
> optional.

I wonder why you want it optional... so I'm going to guess and cover
both cases I can think of below to head off another round of reply on
this point (sorry if this sucks eggs.)

I don't see it as being optional, because it's going to be cheap to run
in the case of a system which has very few or no errors - which is what
you should have for production systems, right?

Remember, only devices and drivers that are present and have been
probed once get added to the deferred probe list, not devices for
which their drivers are modules.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alternative approach to solve the deferred probe

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 08:36:23AM -0700, Frank Rowand wrote:
> On 10/21/2015 1:18 AM, Russell King - ARM Linux wrote:
> > On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
> >> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
> 
> < snip >
> 
> >>> +
> >>>  static bool driver_deferred_probe_enable = false;
> >>> +
> >>>  /**
> >>>   * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
> >>>   *
> >>> @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
> >>>   driver_deferred_probe_trigger();
> >>
> >> Couldn't you put the "driver_deferred_probe_report = true" here?  And then
> >> not add another round of probes.
> > 
> > The idea is not to report anything for drivers that were deferred
> > during the normal bootup.  The above is part of the normal bootup,
> > and the deferred activity should not be warned about.
> 
> The above is currently the last point for probe to succeed or defer
> (until possibly, as you mentioned, module loading resolves the defer).
> If a probe defers above, it will defer again below.  The set of defers
> should be exactly the same above and below.

Why should it?  Isn't this late_initcall() the first opportunity that
deferred devices get to be re-probed from their first set of attempts
via the drivers having their initcalls called?

If what you're saying is true, what's the point of this late_initcall()?



-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alternative approach to solve the deferred probe

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
> > On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> >> Hi Russell,
> >>
> >> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> >> <li...@arm.linux.org.uk> wrote:
> >>>>> What you can do is print those devices which have failed to probe at
> >>>>> late_initcall() time - possibly augmenting that with reports from
> >>>>> subsystems showing what resources are not available, but that's only
> >>>>> a guide, because of the "it might or might not be in a kernel module"
> >>>>> problem.
> >>>>
> >>>> Well, adding those reports would give you a changelog similar to the
> >>>> one in this series...
> >>>
> >>> I'm not sure about that, because what I was thinking of is adding
> >>> a flag which would be set at late_initcall() time prior to running
> >>> a final round of deferred device probing.
> >>
> >> Which round is the final round?
> >> That's the one which didn't manage to bind any new devices to drivers,
> >> which is something you only know _after_ the round has been run.
> >>
> >> So I think we need one extra round to handle this.
> >>
> >>> This flag would then be used in a deferred_warn() printk function
> >>> which would normally be silent, but when this flag is set, it would
> >>> print the reason for the deferral - and this would replace (or be
> >>> added) to the subsystems and drivers which return -EPROBE_DEFER.
> >>>
> >>> That has the effect of hiding all the deferrals up until just before
> >>> launching into userspace, which should then acomplish two things -
> >>> firstly, getting rid of the rather useless deferred messages up to
> >>> that point, and secondly printing the reason why the remaining
> >>> deferrals are happening.
> >>>
> >>> That should be a small number of new lines plus a one-line change
> >>> in subsystems and drivers.
> >>
> >> Apart from the extra round we probably can't get rid of, that sounds OK to 
> >> me.
> > 
> > Something like this.  I haven't put a lot of effort into it to change all
> > the places which return an -EPROBE_DEFER, and it also looks like we need
> > some helpers to report when we have only an device_node (or should that
> > be fwnode?)  See the commented out of_warn_deferred() in
> > drivers/gpio/gpiolib-of.c.  Adding this stuff in the subsystems searching
> > for resources should make debugging why things are getting deferred easier.
> > 
> > We could make driver_deferred_probe_report something that can be
> > deactivated again after the last deferred probe run, and provide the
> > user with a knob that they can turn it back on again.
> > 
> > I've tried this out on two of my platforms, including forcing
> > driver_deferred_probe_report to be enabled, and I get exactly one
> > deferred probe, so not a particularly good test.
> > 
> > The patch won't apply as-is to mainline for all files; it's based on my
> > tree which has some 360 additional patches (which seems to be about
> > normal for my tree now.)
> 
> I like the concept (I have been thinking along similar lines lately).
> But I think this might make the console messages more confusing than
> they are now.

If messages end up being given from the subsystem rather than the driver,
surely they become more consistent?

> The problem is that debug, warn, and error messages
> come from a somewhat random set of locations at the moment.  Some
> come from the driver probe routines and some come from the subsystems
> that the probe routines call.  So the patch is suppressing some
> messages, but not others.

The patch is not complete (read the description above).

> > +void dev_warn_deferred(struct device *dev, const char *fmt, ...)
> > +{
> > +   if (driver_deferred_probe_report) {
> > +   struct va_format vaf;
> > +   va_list ap;
> > +
> > +   va_start(ap, fmt);
> > +   vaf.fmt = fmt;
> > +   vaf.va = 
> > +
> > +   dev_warn(dev, "deferring probe: %pV", );
> > +   va_end(ap);
> > +   }
> > +}
> > +EXPORT_SYMBOL_GPL(dev_warn_deferred);
> 
> The places where dev_warn_deferred() replaces dev_dbg(), we lose the
> ability to turn on debugging and observe the driver reporting th

Re: Alternative approach to solve the deferred probe (was: [GIT PULL] On-demand device probing)

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
> 
> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> >> > What you can do is print those devices which have failed to probe at
> >> > late_initcall() time - possibly augmenting that with reports from
> >> > subsystems showing what resources are not available, but that's only
> >> > a guide, because of the "it might or might not be in a kernel module"
> >> > problem.
> >>
> >> Well, adding those reports would give you a changelog similar to the
> >> one in this series...
> >
> > I'm not sure about that, because what I was thinking of is adding
> > a flag which would be set at late_initcall() time prior to running
> > a final round of deferred device probing.
> 
> Which round is the final round?
> That's the one which didn't manage to bind any new devices to drivers,
> which is something you only know _after_ the round has been run.
> 
> So I think we need one extra round to handle this.
> 
> > This flag would then be used in a deferred_warn() printk function
> > which would normally be silent, but when this flag is set, it would
> > print the reason for the deferral - and this would replace (or be
> > added) to the subsystems and drivers which return -EPROBE_DEFER.
> >
> > That has the effect of hiding all the deferrals up until just before
> > launching into userspace, which should then acomplish two things -
> > firstly, getting rid of the rather useless deferred messages up to
> > that point, and secondly printing the reason why the remaining
> > deferrals are happening.
> >
> > That should be a small number of new lines plus a one-line change
> > in subsystems and drivers.
> 
> Apart from the extra round we probably can't get rid of, that sounds OK to me.

Something like this.  I haven't put a lot of effort into it to change all
the places which return an -EPROBE_DEFER, and it also looks like we need
some helpers to report when we have only an device_node (or should that
be fwnode?)  See the commented out of_warn_deferred() in
drivers/gpio/gpiolib-of.c.  Adding this stuff in the subsystems searching
for resources should make debugging why things are getting deferred easier.

We could make driver_deferred_probe_report something that can be
deactivated again after the last deferred probe run, and provide the
user with a knob that they can turn it back on again.

I've tried this out on two of my platforms, including forcing
driver_deferred_probe_report to be enabled, and I get exactly one
deferred probe, so not a particularly good test.

The patch won't apply as-is to mainline for all files; it's based on my
tree which has some 360 additional patches (which seems to be about
normal for my tree now.)

 drivers/base/dd.c   | 29 +
 drivers/base/power/domain.c |  7 +--
 drivers/clk/clkdev.c|  9 -
 drivers/gpio/gpiolib-of.c   |  5 +
 drivers/gpu/drm/bridge/dw_hdmi.c|  2 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c |  2 +-
 drivers/gpu/drm/imx/imx-ldb.c   |  5 +++--
 drivers/gpu/drm/msm/dsi/dsi.c   |  2 +-
 drivers/gpu/drm/msm/msm_drv.c   |  3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  3 ++-
 drivers/of/irq.c|  5 -
 drivers/pci/host/pci-mvebu.c|  1 +
 drivers/pinctrl/core.c  |  5 +++--
 drivers/pinctrl/devicetree.c|  4 ++--
 drivers/regulator/core.c|  5 +++--
 include/linux/device.h  |  1 +
 16 files changed, 71 insertions(+), 17 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb4639128..bb12224f2901 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -129,7 +129,29 @@ void driver_deferred_probe_del(struct device *dev)
mutex_unlock(_probe_mutex);
 }
 
+static bool driver_deferred_probe_report;
+
+/**
+ * dev_warn_deferred() - report why a probe has been deferred
+ */
+void dev_warn_deferred(struct device *dev, const char *fmt, ...)
+{
+   if (driver_deferred_probe_report) {
+   struct va_format vaf;
+   va_list ap;
+
+   va_start(ap, fmt);
+   vaf.fmt = fmt;
+   vaf.va = 
+
+   dev_warn(dev, "deferring probe: %pV", );
+   va_end(ap);
+   }
+}
+EXPORT_SYMBOL_GPL(dev_warn_deferred);
+
 static bool driver_deferred_probe_enable = false;
+
 /**
  * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
  *
@@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible b

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 10:44:41AM +0100, David Woodhouse wrote:
> On Sun, 2015-10-18 at 20:53 +0100, Mark Brown wrote:
> > On Sun, Oct 18, 2015 at 12:37:57PM -0700, Greg Kroah-Hartman wrote:
> > > On Sun, Oct 18, 2015 at 08:29:31PM +0100, Mark Brown wrote:
> > > > On Fri, Oct 16, 2015 at 11:57:50PM -0700, Greg Kroah-Hartman wrote:
> > 
> > > > > I can't see adding calls like this all over the tree just to solve a
> > > > > bus-specific problem, you are adding of_* calls where they aren't
> > > > > needed, or wanted, at all.
> > 
> > > > This isn't bus specific, I'm not sure what makes you say that?
> > 
> > > You are making it bus-specific by putting these calls all over the tree
> > > in different bus subsystems semi-randomly for all I can determine.
> > 
> > Do you mean firmware rather than bus here?  I think that's the confusion
> > I have...
> 
> Certainly, if it literally is adding of_* calls then that would seem to
> be gratuitously firmware-specific. Nothing should be using those these
> days; any new code should be using the generic device property APIs
> (except in special cases).

I asked Linus Walleij about that with the fwnode_get_named_gpiod() stuff,
and Linus didn't seem to know how this should be used.

It doesn't help that dev->fwnode is not initialised, but dev->of_node
is.  Are we supposed to grope around in dev->of_node for the embedded
fwnode instead of using dev->fwnode?

At the moment, at least to me, fwnode looks like some kind of
experimental half-baked thing rather than a real usable solution.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 02:34:22PM +0200, Tomeu Vizoso wrote:
> ... If a device is available and has
> a compatible driver, but it cannot be probed because a dependency
> isn't going to be available, that's an error and is going to cause
> real-world problems unless the device is redundant. Currently we say
> nothing because with deferred probe the probe callbacks are also part
> of the mechanism that determines the dependency order.

So what if device X depends on device Y, and we have a driver for
device Y built-in to the kernel, but the driver for device X is a
module?

I don't see this being solvable in the way you describe above - it's
going to identify X as being unable to be satisfied, and report it as
an error - but it's not an error at all.

> Having a specific switch for enabling deferred probe logging sounds
> good, but there's going to be hundreds of spurious messages about
> deferred probes that were just deferrals and only one of them is going
> to be the actual error in which a device failed to find a dependency.

Why would there be?  Sounds like something's very wrong there.

You should only get deferred probes for devices which are declared to
be present, but their resources have not yet been satisfied.  It
doesn't change anything if you have a kernel with lots of device drivers
or just the device drivers you need - the device drivers you don't need
do not contribute to the deferred probing in any way.

So, really, after boot and all appropriate modules have been loaded,
you should end up with no deferred probes.  Are you saying that you
still have "hundreds" at that point?  If you do, that sounds like
there's something very wrong.

> 3) Regarding total boot time, I don't expect this series to make much
> of a difference because though we would save a lot of matching and
> querying for resources, that's little time compared with how long we
> wait for hardware to react during probing. Async probing is more
> likely to help with drivers that take a long time to probe.

For me, on my fastest ARM board, though running serial console:

[2.293468] VFS: Mounted root (ext4 filesystem) on device 179:1.

There's a couple of delays in there, but they're not down to deferred
probing.  The biggest one is serial console startup (due to the time
it takes to write out the initial kernel messages):

[0.289962] f1012000.serial: ttyS0 at MMIO 0xf1012000 (irq = 23, base_baud = 
15625000) is a 16550A
[0.944124] console [ttyS0] enabled

and DSA switch initialisation:

[1.530655] libphy: dsa slave smi: probed
[2.034426] dsa dsa@0 lan6 (uninitialized): attached PHY at address 0 
[Generic PHY]

I'm not sure what causes that, but at a guess it's having to talk to the
DSA switch over the MDIO bus via several layers of indirect accesses.
Of course, serial console adds to the boot time significantly anyway,
especially at the "standard" kernel logging level.

> One more thing about the breakage we have seen so far is that it's
> generally caused by implicit dependencies and hunting those is
> probably the second biggest timesink of the linux embedded developer
> after failed probes.

... which is generally caused by the crappy code which the average
embedded Linux developer creates, particularly with the crappy error
messages they like creating.  For the most part, they _might_ as well
just print "Error!\n" and be done with it, for all the use they are.
When creating an error print, your average embedded Linux developer
fails to print the _reason_ why something failed, which makes debugging
it much harder.

The first thing I do when I touch code that needs this kind of debugging
is to go through and add printing of the error code.  That normally lets
me quickly narrow down what's failed.

If embedded Linux developers are struggling with this, they only have
themselves to blame.

In the case of deferred probing, what _may_ help is if we got rid of the
core code printing that driver X requested deferred probing, instead
moving the responsibility to report this onto the driver or subsystem.
Resource claiming generally has the struct device, and can use dev_warn()
to report which device is being probed, along with which resource is
not yet available.

This debug problem is solvable without needing to resort to complex
probing solutions.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 04:10:56PM +0200, Tomeu Vizoso wrote:
> On 19 October 2015 at 15:18, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> > On Mon, Oct 19, 2015 at 02:34:22PM +0200, Tomeu Vizoso wrote:
> >> ... If a device is available and has
> >> a compatible driver, but it cannot be probed because a dependency
> >> isn't going to be available, that's an error and is going to cause
> >> real-world problems unless the device is redundant. Currently we say
> >> nothing because with deferred probe the probe callbacks are also part
> >> of the mechanism that determines the dependency order.
> >
> > So what if device X depends on device Y, and we have a driver for
> > device Y built-in to the kernel, but the driver for device X is a
> > module?
> >
> > I don't see this being solvable in the way you describe above - it's
> > going to identify X as being unable to be satisfied, and report it as
> > an error - but it's not an error at all.
> 
> It's going to probe Y at late_initcall, then probe X when its driver
> is registered. No deferred probes nor messages about it.
> 
> But if you meant to write the opposite case (X built-in and Y in a
> module), then I have to ask you in what situation that would make
> sense.

I did mean the opposite way around.  It may not make sense if you're
targetting a single platform, but it may make sense in a single zImage
kernel.

Consider something like a single zImage kernel that is built with
everything built-in to be able to boot and mount rootfs without
initramfs support on both platform A and platform B.  Both platforms
share some hardware (eg, an I2C GPIO expander) which is built as a
module.  It is a resource provider.  Platform B contains a driver
which is required to boot on platform A, but not platform B (so the
kernel has to have that driver built-in.)  On platform B, there is
a dependency to the I2C GPIO expander device.

> >> Having a specific switch for enabling deferred probe logging sounds
> >> good, but there's going to be hundreds of spurious messages about
> >> deferred probes that were just deferrals and only one of them is going
> >> to be the actual error in which a device failed to find a dependency.
> >
> > Why would there be?  Sounds like something's very wrong there.
> 
> Sorry about that, I have checked that only now and I "only" get 39
> deferred probe messages on exynos5250-snow.

I typically see one or two, maybe five maximum on the platforms I have
here, but normally zero.

> > So, really, after boot and all appropriate modules have been loaded,
> > you should end up with no deferred probes.  Are you saying that you
> > still have "hundreds" at that point?  If you do, that sounds like
> > there's something very wrong.
> 
> I was talking about messages if we log each -EPROBE_DEFER, not devices
> that remain to be probed. The point being that right now we don't have
> a way to know if we are deferring because the dependency will be
> around later, or if we have a problem and the dependency isn't going
> to be there at all.

What's the difference between a dependency which isn't around because
the driver is not built into the kernel but is available as a module,
and a dependency that isn't around because the module hasn't been
loaded yet?

How do you distinguish between those two scenarios?  In the former
scenario, the device will eventually come up when udev loads the
module.  In the latter case, it's a persistent failing case.

> Agreed, with the note from above on why it would be better to only
> print such a message only when the -EPROBE_DEFER is likely to be a
> problem.

... and my argument is that there's _no way_ to know for certain which
deferred probes will be a problem, and which won't.  The only way to
definitely know that is if you disable kernel modules, and require
all drivers to be built into the kernel.

What you can do is print those devices which have failed to probe at
late_initcall() time - possibly augmenting that with reports from
subsystems showing what resources are not available, but that's only
a guide, because of the "it might or might not be in a kernel module"
problem.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 05:00:54PM +0200, Tomeu Vizoso wrote:
> On 19 October 2015 at 16:30, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> > I typically see one or two, maybe five maximum on the platforms I have
> > here, but normally zero.
> 
> Hmm, I have given a look at our lava farm and have seen 2 dozens as
> common (with multi_v7).

No, because the lava farms tend not to be public.

> > What you can do is print those devices which have failed to probe at
> > late_initcall() time - possibly augmenting that with reports from
> > subsystems showing what resources are not available, but that's only
> > a guide, because of the "it might or might not be in a kernel module"
> > problem.
> 
> Well, adding those reports would give you a changelog similar to the
> one in this series...

I'm not sure about that, because what I was thinking of is adding
a flag which would be set at late_initcall() time prior to running
a final round of deferred device probing.

This flag would then be used in a deferred_warn() printk function
which would normally be silent, but when this flag is set, it would
print the reason for the deferral - and this would replace (or be
added) to the subsystems and drivers which return -EPROBE_DEFER.

That has the effect of hiding all the deferrals up until just before
launching into userspace, which should then acomplish two things -
firstly, getting rid of the rather useless deferred messages up to
that point, and secondly printing the reason why the remaining
deferrals are happening.

That should be a small number of new lines plus a one-line change
in subsystems and drivers.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 04:29:40PM +0100, David Woodhouse wrote:
> I don't know that there *is* a coherent plan here to address it all.
> 
> Certainly, we *will* need subsystems to have firmware-specific
> knowledge in some cases. Take GPIO as an example; ACPI *has* a way to
> describe GPIO, and properties which reference GPIO pins are intended to
> work through that — while in DT, properties which reference GPIO pins
> will have different contents. They'll be compatible at the driver
> level, in the sense that there's a call to get a given GPIO given the
> property name, but the subsystems *will* be doing different things
> behind the scenes.

It's a bit ironic that you've chosen GPIO as an example there.  The
"new" GPIO API (the gpiod_* stuff) only has a fwnode way to get the
gpio descriptor.  There's no of_* method.

I'd like to use the gpiod_* stuff, but I feel that my options are
rather limited: either use fwnode_get_named_gpiod() with
>of_node->fwnode, which seems like a hack by going underneath
the covers of how fwnode is (partially) implemented with DT, or by
using of_get_named_gpio() and the converting the gpio number to a
descriptor via gpio_to_desc().  Both feel very hacky.

If ACPI already handles GPIOs internally, then I'm left wondering
why GPIO descriptor stuff went down the fwnode route at all - it
seems rather pointless in this case, and it seems to make the use
of the gpiod* interfaces where we _do_ need to use it (DT) harder
and more hacky.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 08:27:44PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Oct 19, 2015 at 04:43:24PM +0100, Russell King - ARM Linux wrote:
> > It's a bit ironic that you've chosen GPIO as an example there.  The
> > "new" GPIO API (the gpiod_* stuff) only has a fwnode way to get the
> > gpio descriptor.  There's no of_* method.
> 
> Without following all that fwnode discussion:
> gpiod_get et al. should work for you here, doesn't it? It just takes a
> struct device * and I'm happy with it.

What if you don't have a struct device?  I had that problem recently
when modifying the mvebu PCIe code.  The 'struct device' node doesn't
contain the GPIOs, it's the PCIe controller.  Individual ports on the
controller are described in DT as sub-nodes, and the sub-nodes can
have a GPIO for card reset purposes.  These sub-nodes don't have a
struct device.

Right now, I'm having to do this to work around this issue:

reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, );
if (reset_gpio == -EPROBE_DEFER) {
ret = reset_gpio;
goto err;
}

if (gpio_is_valid(reset_gpio)) {
unsigned long gpio_flags;

port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
  port->name);
if (!port->reset_name) {
ret = -ENOMEM;
goto err;
}

if (flags & OF_GPIO_ACTIVE_LOW) {
dev_info(dev, "%s: reset gpio is active low\n",
 of_node_full_name(child));
gpio_flags = GPIOF_ACTIVE_LOW |
 GPIOF_OUT_INIT_LOW;
} else {
gpio_flags = GPIOF_OUT_INIT_HIGH;
}

ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
port->reset_name);
if (ret) {
if (ret == -EPROBE_DEFER)
goto err;
goto skip;
}

port->reset_gpio = gpio_to_desc(reset_gpio);
}

Not nice, is it?  Not nice to have that in lots of drivers either.

However, switching to use any of_* or fwnode_* thing also carries with
it another problem: you can't control the name appearing in the
allocation, so you end up with a bunch of GPIOs requested with a "reset"
name - meaning you lose any identification of which port the GPIO was
bound to.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] On-demand device probing

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
> 
> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> >> > What you can do is print those devices which have failed to probe at
> >> > late_initcall() time - possibly augmenting that with reports from
> >> > subsystems showing what resources are not available, but that's only
> >> > a guide, because of the "it might or might not be in a kernel module"
> >> > problem.
> >>
> >> Well, adding those reports would give you a changelog similar to the
> >> one in this series...
> >
> > I'm not sure about that, because what I was thinking of is adding
> > a flag which would be set at late_initcall() time prior to running
> > a final round of deferred device probing.
> 
> Which round is the final round?

I said "a" not "the".  Maybe I should've said "one last round of deferred
probing before entering userspace".

> That's the one which didn't manage to bind any new devices to drivers,
> which is something you only know _after_ the round has been run.
> 
> So I think we need one extra round to handle this.

Yes - because the idea is that we do everything we do today without
printing anything about deferred probes.  We then set a flag which
indicates we should report defers, and then we trigger another round
of probes.

If everything probed successfully, the deferred probe list will be
empty and nothing will be seen.  Otherwise, we should end up with a
report of all the devices that weren't able to be bound to their
drivers due to -EPROBE_DEFER.

> 
> > This flag would then be used in a deferred_warn() printk function
> > which would normally be silent, but when this flag is set, it would
> > print the reason for the deferral - and this would replace (or be
> > added) to the subsystems and drivers which return -EPROBE_DEFER.
> >
> > That has the effect of hiding all the deferrals up until just before
> > launching into userspace, which should then acomplish two things -
> > firstly, getting rid of the rather useless deferred messages up to
> > that point, and secondly printing the reason why the remaining
> > deferrals are happening.
> >
> > That should be a small number of new lines plus a one-line change
> > in subsystems and drivers.
> 
> Apart from the extra round we probably can't get rid of, that sounds
> OK to me.

Yes, it means a little longer to boot, but if there's nothing pending
this _should_ only be microseconds - it should be nothing more than
setting the flag, possibly taking and releasing a lock, and checking
that the deferred probe list is empty.

Of course, if the deferred probe list isn't empty, then there'll be
more expense - but I'm willing to bet that for those developers with
serial console enabled, the kernel boot will be faster overall due
to the reduced number of characters printed during the boot.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression: USB OTG port breaks after a few hours in host mode on iMX6

[Russell King - ARM Linux]

On Thu, Oct 08, 2015 at 09:52:52AM +, Peter Chen wrote:
> I can't reproduce it for 5 hours, and will change pinmux (do the same
> thing with your platform), and do the overnight test.

There's definitely something weird going on.  Over night last night,
I left the Logitech universal receiver in the port, and this morning
it was indicating in /proc/interrupts:

283: 50  0  0  0   GPC  43 Edge  
2184000.usb

I removed it, and now I have:

283: 50  0   1716  0   GPC  43 Edge  
2184000.usb

which is increasing at a rate of 90 per minute.

Nothing in the kernel message log indicating why this may be.  It looks
like runtime PM doesn't work on this port:

/sys/bus/platform/devices/2184000.usb/power/runtime_active_time:109850496
/sys/bus/platform/devices/2184000.usb/power/runtime_status:active
/sys/bus/platform/devices/2184000.usb/power/runtime_suspended_time:0

whereas the other port (which has zero interrupts) it does:

/sys/bus/platform/devices/2184200.usb/power/runtime_active_time:16924
/sys/bus/platform/devices/2184200.usb/power/runtime_status:suspended
/sys/bus/platform/devices/2184200.usb/power/runtime_suspended_time:109861760

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regression: USB OTG port breaks after a few hours in host mode on iMX6

[Russell King - ARM Linux]

This bug is soo obscure, I'm not even sure how to start debugging it.
This never used to be a problem, but I'm not sure when it started as
USB (in general) is not something I use regularly.

In this setup, the USB OTG port is wired in host mode via pinmix
configuration of the USB OTG ID pin:

MX6QDL_PAD_GPIO_1__USB_OTG_ID 0x13059

The problem characterised so far: after booting the kernel, USB seems
to work fine.  You can plug and unplug devices from both USB host and
USB OTG ports, and it's detected.

If you boot a kernel with nothing plugged in, leave it for maybe four
hours, then plug in a device to either port, the device will be seen
in the USB host port, but completely ignored in the USB OTG port.
Both USB OTG ports have power, confirmed last night when using a USB
microscope with built-in LED lighting: the LEDs are functional, the
device is otherwise completely ignored.

The above is basically what I know so far: I don't know when the port
fails, whether in the case of "boot, wait four hours, and it's dead"
whether it's dead from boot: when I've tried testing it immediately
after boot, it's worked.

As it takes around four hours to reproduce, and I have a massive patch
stack on top of the kernel for this platform, I'm unwilling to attempt
a git bisection to try and find when this occurred, but I know it's
been going on for a while now as I've noticed the same behaviour when
I transfer my logitech USB receiver to that port.

I had thought it was a power issue, but the USB camera last night
confirmed that it's a port driver issue.

Certainly earlier 3.x kernels did not have this behaviour.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Russell King - ARM Linux]

On Tue, Sep 01, 2015 at 02:54:17PM +0300, Mathias Nyman wrote:
> On 31.08.2015 21:58, Duc Dang wrote:
> >On Thu, Aug 20, 2015 at 12:38 PM, Duc Dang  wrote:
> >>The xhci platform driver needs to work on systems that
> >>either only support 64-bit DMA or only support 32-bit DMA.
> >>Attempt to set a coherent dma mask for 64-bit DMA, and
> >>attempt again with 32-bit DMA if that fails.
> >>
> >>[dhdang: regenerate the patch over 4.2-rc5 and address new comments]
> >>Signed-off-by: Mark Langsdorf 
> >>Tested-by: Mark Salter 
> >>Signed-off-by: Duc Dang 
> >>
> >>---
> >>Changes from v6:
> >> -Add WARN_ON if dma_mask is NULL
> >> -Use dma_coerce_mask_and_coherent to assign
> >> dma_mask and coherent_dma_mask
> >>
> >>Changes from v5:
> >> -Change comment
> >> -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
> >> to make sure dma_set_mask_and_coherent does not fail prematurely.
> >>
> >>Changes from v4:
> >> -None
> >>
> >>Changes from v3:
> >> -Re-generate the patch over 4.2-rc5
> >> -No code change.
> >>
> >>Changes from v2:
> >> -None
> >>
> >>Changes from v1:
> >> -Consolidated to use dma_set_mask_and_coherent
> >> -Got rid of the check against sizeof(dma_addr_t)
> >>
> >>  drivers/usb/host/xhci-plat.c | 21 +
> >>  1 file changed, 13 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> >>index 890ad9d..e4c7f9d 100644
> >>--- a/drivers/usb/host/xhci-plat.c
> >>+++ b/drivers/usb/host/xhci-plat.c
> >>@@ -93,14 +93,19 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >> if (irq < 0)
> >> return -ENODEV;
> >>
> >>-   /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> >>-   ret = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
> >>-   if (ret)
> >>-   return ret;
> >>-   if (!pdev->dev.dma_mask)
> >>-   pdev->dev.dma_mask = >dev.coherent_dma_mask;
> >>-   else
> >>-   dma_set_mask(>dev, DMA_BIT_MASK(32));
> >>+   /* Throw a waring if broken platform code didn't initialize 
> >>dma_mask */
> >>+   WARN_ON(!pdev->dev.dma_mask);
> >>+   /*
> >>+* Try setting dma_mask and coherent_dma_mask to 64 bits,
> >>+* then try 32 bits
> >>+*/
> >>+   ret = dma_coerce_mask_and_coherent(>dev, DMA_BIT_MASK(64));
> >>+   if (ret) {
> >>+   ret = dma_coerce_mask_and_coherent(>dev,
> >>+  DMA_BIT_MASK(32));
> >>+   if (ret)
> >>+   return ret;
> >>+   }

This isn't very good.  If dev.dma_mask is already set,
dma_coerce_mask_and_coherent() will always overwrite it.  There's also
no need to call it twice.  This, imho, is much better:

/* Try to set a 64-bit DMA mask first */
if (WARN_ON(!pdev->dev.dma_mask)) {
/* Eek, platform didn't initialise the streaming DMA mask */
ret = dma_coerce_mask_and_coherent(>dev, 
DMA_BIT_MASK(64));
} else {
ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(64));
}

/* If that failed, fall back to a 32-bit DMA mask */
if (ret) {
ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
if (ret)
return ret;
}

since it preserves the dev.dma_mask pointer if it was properly setup

Really, drivers shouldn't be messing around with that pointer - especially
if it's already been correctly setup.  A platform may require separate
streaming and coherent masks, and we should respect that.

(The whole dma_mask being a pointer thing is a left-over from the PCI
layer which has never been cleaned up through fear of breaking something.)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 08:18:48PM -0700, Duc Dang wrote:
 diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
 index 890ad9d..5d03f8b 100644
 --- a/drivers/usb/host/xhci-plat.c
 +++ b/drivers/usb/host/xhci-plat.c
 @@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
   if (irq  0)
   return -ENODEV;
  
 - /* Initialize dma_mask and coherent_dma_mask to 32-bits */
 - ret = dma_set_coherent_mask(pdev-dev, DMA_BIT_MASK(32));
 - if (ret)
 - return ret;
 - if (!pdev-dev.dma_mask)
 - pdev-dev.dma_mask = pdev-dev.coherent_dma_mask;
 - else
 - dma_set_mask(pdev-dev, DMA_BIT_MASK(32));
 + /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
 + ret = dma_set_mask_and_coherent(pdev-dev, DMA_BIT_MASK(64));
 + if (ret) {
 + ret = dma_set_mask_and_coherent(pdev-dev, DMA_BIT_MASK(32));
 + if (ret)
 + return ret;
 + }

Note that dma_set_mask_and_coherent() and the original code are not
equivalent because of this:

if (!pdev-dev.dma_mask)
pdev-dev.dma_mask = pdev-dev.coherent_dma_mask;

If we know that pdev-dev.dma_mask will always be initialised at this
point, then the above change is fine.  If not, it's introducing a
regression - dma_set_mask_and_coherent() will fail if pdev-dev.dma_mask
is NULL (depending on the architectures implementation of dma_set_mask()).

Prefixing the above change with the two lines I mention above would
ensure equivalent behaviour.  Even if we do want to get rid of this,
I'd advise to do it as a separate patch after this change, which can
be independently reverted if there's problems with its removal.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-am33-list] [PATCH v2 0/7] fix build failure of mn10300

[Russell King - ARM Linux]

On Thu, Jun 18, 2015 at 10:31:07AM -0500, Jay C. Polmar wrote:
 I am on this list by mistake and 5/15/2011 we requested to be removed,
 can someone remove me from this list?

There are six mailing lists in the Cc line.  For all of the lists
present there, I can't help you, but you should be able to unsubscribe
yourself.  Look at the footer on any message you receive from the list
and it should tell you how to get more information on that subject.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] fix build failure of mn10300

[Russell King - ARM Linux]

On Thu, Jun 18, 2015 at 05:47:46PM +0530, Sudip Mukherjee wrote:
 This is an attempt to fix the build failures when building mn10300 with
 allmodconfig. As I have never worked with arch files so I hope you will
 point me to right direction to correct my mistakes in this attempt.

Why has the linux-arm-kernel mailing list been copied for something
which clearly has nothing to do with ARM?  Am I missing something
in this series?

(Or is there a competition on to see who can add as many entries in
the Cc without getting caught by any filtering? :) )

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: renesas: fix extcon dependency

[Russell King - ARM Linux]

On Wed, Jan 28, 2015 at 10:47:18PM +0100, Arnd Bergmann wrote:
 The renesas usbhs driver calls extcon_get_edev_by_phandle(), which
 is defined in drivers/extcon/extcon-class.c, and that can be a
 loadable module. If the extcon-class support is disabled, usbhs
 will work correctly for all devices that do not need extcon.
 
 However, if extcon-class is a loadable module, and usbhs is
 built-in, the kernel fails to link. In order to solve that,
 we need a Kconfig dependency that allows extcon to be disabled
 but does not allow usbhs built-in if extcon is a module.
 
 Signed-off-by: Arnd Bergmann a...@arndb.de
 
 diff --git a/drivers/usb/renesas_usbhs/Kconfig 
 b/drivers/usb/renesas_usbhs/Kconfig
 index de83b9d0cd5c..0ea9040b9f10 100644
 --- a/drivers/usb/renesas_usbhs/Kconfig
 +++ b/drivers/usb/renesas_usbhs/Kconfig
 @@ -6,6 +6,7 @@ config USB_RENESAS_USBHS
   tristate 'Renesas USBHS controller'
   depends on USB_GADGET
   depends on ARCH_SHMOBILE || SUPERH || COMPILE_TEST
 + depends on EXTCON || !EXTCON # for module build if extcon is

The comment doesn't make much sense on its own (and when I read it before
reading the above description, it was down right confusing.)  Maybe a
longer comment would be a good idea?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Lockdep problem: v3.18+ (with yesterday's Linus tip - 6f51ee709e4c)

[Russell King - ARM Linux]

On Wed, Jan 07, 2015 at 10:42:14AM -0500, Alan Stern wrote:
 On Thu, 18 Dec 2014, Greg Kroah-Hartman wrote:
  That seems reasonable to me, unbinding when a reset is happening is
  going to be a rare condition, but if we get rid of it, and we try to
  queue a reset for a device that is gone, we will just fail the reset,
  right?  If all should be fine, I have no objection to removing it.
 
 Russell, can you reproduce that lockdep violation whenever you want?

I can certainly try it - I move the logitek receiver around between about
five machines depending on which I'm wanting to use.  Obviously, having
had the Christmas holidays recently, it hasn't been moved so much.

However, at the moment, I'm still not doing much in the way of kernel
work due to ongoing illness.

 If you can, does the following patch help?

I'll give it a go once I've worked out how reproducable it is, many
thanks for looking into this.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

3.18 / 3.19-rc1: ehci scheduling bug?

[Russell King - ARM Linux]

While testing an EM28xx based USB DVB stick which I've recently acquired,
I find that occasionally the driver stops responding after it returns
-EFBIG from one of its ioctls.  I've no idea whether there's a previous
kernel version which works.

Finding the EFBIG return in ehci-sched.c, I decided to make the debug
a little more verbose there, and got:

ci_hdrc ci_hdrc.0: request e7668800 would overflow (4093+63 = 4096)

So, I augmented it, which revealed:

ci_hdrc ci_hdrc.0: request ecd75000 would overflow (4090+64-1 = 4096+0)
ci_hdrc ci_hdrc.0: hs 1 xfer_flag 0x206 empty 1 new_stream 1 now 1032 next 4090 
base 1040

Here, start = 4090, period = 1, span = 64, mod = 4096, wrap = 0.

This looks fairly complex to debug - any suggestions on how to debug
this further?

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 3.18 / 3.19-rc1: ehci scheduling bug?

[Russell King - ARM Linux]

On Wed, Dec 24, 2014 at 12:00:00PM +0100, Frank Schäfer wrote:
 See
 https://bugzilla.kernel.org/show_bug.cgi?id=72891
 
 Solved by
 http://www.spinics.net/lists/linux-usb/msg118366.html

Thanks, that seems to solve it.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lockdep problem: v3.18+ (with yesterday's Linus tip - 6f51ee709e4c)

[Russell King - ARM Linux]

While unplugging a Logitek Keyboard/mouse micro-receiver, I got the
lockdep splat below.

However, I don't fully understand this splat - I see nothing in
flush_work() nor process_one_work() making use of intf-reset_ws -
which seems to be a USB thing.  I guess lockdep is being re-used to
validate work stuff, and lock is just plain misleading.

usb 2-1.1: USB disconnect, device number 3

=
[ INFO: possible recursive locking detected ]
3.18.0+ #1459 Not tainted
-
kworker/0:1/2758 is trying to acquire lock:
 ((intf-reset_ws)){+.+.+.}, at: [c003ba90] flush_work+0x0/0x264

but task is already holding lock:
 ((intf-reset_ws)){+.+.+.}, at: [c003ca40] process_one_work+0x130/0x4b4

other info that might help us debug this:
 Possible unsafe locking scenario:

   CPU0
   
  lock((intf-reset_ws));
  lock((intf-reset_ws));

 *** DEADLOCK ***

 May be due to missing lock nesting notation

4 locks held by kworker/0:1/2758:
 #0:  (events){.+.+.+}, at: [c003ca40] process_one_work+0x130/0x4b4
 #1:  ((intf-reset_ws)){+.+.+.}, at: [c003ca40] process_one_work+0x130/0x4b4
 #2:  (dev-mutex){..}, at: [c0438c70] 
usb_lock_device_for_reset+0x58/0xd0
 #3:  (dev-mutex){..}, at: [c038cc10] device_release_driver+0x20/0x34

stack backtrace:
CPU: 0 PID: 2758 Comm: kworker/0:1 Not tainted 3.18.0+ #1459
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Workqueue: events __usb_queue_reset_device
Backtrace:
[c0012228] (dump_backtrace) from [c00123c0] (show_stack+0x18/0x1c)
 r6:c140b5ac r5:c141ce9c r4: r3:e3b230c0
[c00123a8] (show_stack) from [c06d7f00] (dump_stack+0x7c/0x98)
[c06d7e84] (dump_stack) from [c00619a0] (__lock_acquire+0x13f4/0x1bb0)
 r4:c0bd9190 r3:e3b230c0
[c00605ac] (__lock_acquire) from [c00626c0] (lock_acquire+0xb0/0x124)
 r10: r9:c003ba90 r8: r7: r6: r5:ed810670
 r4:
[c0062610] (lock_acquire) from [c003bad4] (flush_work+0x44/0x264)
 r10:ffed r9:c0a43170 r8:ed810400 r7:ed810670 r6:0001 r5:ed810660
 r4:
[c003ba90] (flush_work) from [c003d8f0] (__cancel_work_timer+0x8c/0x124)
 r7:ffe0 r6: r5: r4:ed810660
[c003d864] (__cancel_work_timer) from [c003d9b4] 
(cancel_work_sync+0x14/0x18)
 r7:ed810420 r6:ed810420 r5:c0a43170 r4:ee357068
[c003d9a0] (cancel_work_sync) from [c04476dc] 
(usb_unbind_interface+0x90/0x280)
[c044764c] (usb_unbind_interface) from [c038cb9c] 
(__device_release_driver+078/0xcc)
 r10:ffed r9:000c r8:fff4 r7:ee357000 r6:ed810420 r5:c0a43170
 r4:ed810420
[c038cb24] (__device_release_driver) from [c038cc18] 
(device_release_driver+0x28/0x34)
 r5:ed810420 r4:ed810454
[c038cbf0] (device_release_driver) from [c044794c] 
(usb_driver_release_interface+0x80/0x84)
 r5:0001 r4:ed810400
[c04478cc] (usb_driver_release_interface) from [c0447970] 
(usb_forced_unbind_intf+0x20/0x30)
 r7:ee357000 r6:ed80c000 r5:ed80c054 r4:ed810400
[c0447950] (usb_forced_unbind_intf) from [c04479e4] 
(unbind_marked_interfaces+0x64/0x74)
 r4:0002 r3:0020
[c0447980] (unbind_marked_interfaces) from [c0447b58] 
(usb_unbind_and_rebind_marked_interfaces+0x14/0x20)
 r6:ed80c050 r5: r4:ee357000 r3:006b
[c0447b44] (usb_unbind_and_rebind_marked_interfaces) from [c043c470] 
(usb_reset_device+0x1dc/0x234)
 r4:ed81 r3:006b
[c043c294] (usb_reset_device) from [c0443f6c] 
(__usb_queue_reset_device+0x40/0x58)
 r10:eefb9b00 r9:c0a59178 r8: r7:dd505eb0 r6:ee357000 r5:ee357068
 r4:ed810260
[c0443f2c] (__usb_queue_reset_device) from [c003cad0] 
(process_one_work+0x1c0/0x4b4)
 r6:eefb6500 r5:dd6dde00 r4:ed810260 r3:c0443f2c
[c003c910] (process_one_work) from [c003ce34] (worker_thread+0x34/0x4b0)
 r10:eefb6500 r9:dd6dde00 r8:0008 r7:dd6dde18 r6:eefb6500 r5:0001
 r4:eefb6530
[c003ce00] (worker_thread) from [c0042228] (kthread+0xe0/0xfc)
 r10: r9: r8: r7:c003ce00 r6:dd6dde00 r5:
 r4:dd66e880
[c0042148] (kthread) from [c000ecc8] (ret_from_fork+0x14/0x2c)
 r7: r6: r5:c0042148 r4:dd66e880

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/5] i2c: pxa: prepare/unprepare clocks

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 06:07:40PM +0300, Dmitry Eremin-Solenikov wrote:
 Change clk_enable/disable() calls to clk_prepare_enable() and
 clk_disable_unprepare().
 
 Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com
 ---
  drivers/i2c/busses/i2c-pxa.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
 index be671f7..2e75375 100644
 --- a/drivers/i2c/busses/i2c-pxa.c
 +++ b/drivers/i2c/busses/i2c-pxa.c
 @@ -1297,7 +1297,7 @@ static int i2c_pxa_suspend_noirq(struct device *dev)
   struct platform_device *pdev = to_platform_device(dev);
   struct pxa_i2c *i2c = platform_get_drvdata(pdev);
  
 - clk_disable(i2c-clk);
 + clk_disable_unprepare(i2c-clk);

Since clk_unprepare() and clk_prepare() can sleep, it is unwise to call
these with IRQs disabled - the _noirq variants of these are run with
IRQs disabled.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 1/5] serial: pxa: prepare/unprepare clocks

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 06:07:39PM +0300, Dmitry Eremin-Solenikov wrote:
 Change clk_enable/disable() calls to clk_prepare_enable() and
 clk_disable_unprepare().

NAK.  Console writes can be called from IRQs-off regions.  clk_prepare()
can sleep.  Sleeping is not permitted with IRQs off.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Russell King - ARM Linux]

On Tue, Nov 18, 2014 at 12:05:45AM +0400, Dmitry Eremin-Solenikov wrote:
 Hello,
 
 2014-11-17 21:44 GMT+03:00 Robert Jarzmik robert.jarz...@free.fr:
  Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:
 
  Change clk_enable/disable() calls to clk_prepare_enable() and
  clk_disable_unprepare().
 
  Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com
  ---
   drivers/usb/gadget/udc/pxa25x_udc.c | 8 
   1 file changed, 4 insertions(+), 4 deletions(-)
 
  diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c 
  b/drivers/usb/gadget/udc/pxa25x_udc.c
  index 42f7eeb..e4964ee 100644
  --- a/drivers/usb/gadget/udc/pxa25x_udc.c
  +++ b/drivers/usb/gadget/udc/pxa25x_udc.c
  @@ -103,8 +103,8 @@ static const char ep0name [] = ep0;
 
   /* IXP doesn't yet support linux/clk.h */
   #define clk_get(dev,name)NULL
  -#define clk_enable(clk)  do { } while (0)
  -#define clk_disable(clk) do { } while (0)
  +#define clk_prepare_enable(clk)  do { } while (0)
  +#define clk_disable_unprepare(clk)   do { } while (0)
   #define clk_put(clk) do { } while (0)
 
   #endif
  @@ -932,7 +932,7 @@ static int pullup(struct pxa25x_udc *udc)
if (!udc-active) {
udc-active = 1;
/* Enable clock for USB device */
  - clk_enable(udc-clk);
  + clk_prepare_enable(udc-clk);
 
  Guess what, Russell's remark on i2c and serial made me cross-check.  And 
  there
  is a case where this will be called in irq context too ...
 
  See :
  - do_IRQ
- lubbock_vbus_irq()
  - pxa25x_udc_vbus_session()
- pullup()
  - clk_prepare_enable()
- CRACK
 
  Note that your patch is not really the faulty one, I think a threaded irq
  instead of the raw irq should do the trick. And it is granted on UDC api 
  that
  vbus function is called in a sleeping context (Felipe correct me if I'm
  wrong), so a patch to fix this before your current code would be fine.
 
 OK, I will take a look. It seems the correct way would be to strip this code
 away to a phy, like gpio-vbus or nop phys. Do you have access to lubbock
 (or maybe Daniel, Haojian or Russell has?)?

Robert has my Lubbock now.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Wed, Oct 15, 2014 at 10:25:13PM +0100, Russell King - ARM Linux wrote:
 On Wed, Oct 15, 2014 at 10:23:10PM +0100, Russell King - ARM Linux wrote:
  As I said, I have a patch in progress, but it seems that there needed
  to be some discussion about exactly which compiler versions are affected.
  It seems that it's not as trivial as looking at the GCC bug entry.
 
 ... and in any case, it has been a known bug for well over a year now,
 and it seems that it doesn't affect _that_ many people.  So taking some
 extra time to get it properly correct is the _right_ thing to do.

Well, this is just great.  Pushing out the change which blacklists these
compilers takes out Olof's kernel build system...

Things are not as trivial as they seem.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Tue, Oct 14, 2014 at 04:06:40AM +0200, Greg KH wrote:
 On Mon, Oct 13, 2014 at 12:43:07PM +0100, Russell King - ARM Linux wrote:
  I think the only viable solution here is that:
  
  1. We blacklist the bad compiler versions outright in the kernel.
 
 Yes, please do this, it's what we have done for other buggy compiler
 versions, no need to do something different here.
 
  Remember, it's the distro's choice to fix these buggy compilers, so the
  onus is on _them_ to deal with the mess they've created by doing so.
 
 I totally agree.
 
 Is someone going to send this patch, or do I have to write it myself?

As I said, I have a patch in progress, but it seems that there needed
to be some discussion about exactly which compiler versions are affected.
It seems that it's not as trivial as looking at the GCC bug entry.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Wed, Oct 15, 2014 at 10:23:10PM +0100, Russell King - ARM Linux wrote:
 As I said, I have a patch in progress, but it seems that there needed
 to be some discussion about exactly which compiler versions are affected.
 It seems that it's not as trivial as looking at the GCC bug entry.

... and in any case, it has been a known bug for well over a year now,
and it seems that it doesn't affect _that_ many people.  So taking some
extra time to get it properly correct is the _right_ thing to do.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Mon, Oct 13, 2014 at 09:11:34AM +, David Laight wrote:
 From: Nathan Lynch
  On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
  
   Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
   it seems that this has been known about for some time.)
  
  Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x  3
  are affected, as well as 4.9.0.
  
   We can blacklist these GCC versions quite easily.  We already have GCC
   3.3 blacklisted, and it's trivial to add others.  I would want to include
   some proper details about the bug, just like the other existing entries
   we already have in asm-offsets.c, where we name the functions that the
   compiler is known to break where appropriate.
  
  Before blacklisting anything, it's worth considering that simple version
  checks would break existing pre-4.8.3 compilers that have been patched
  for PR58854.  It looks like Yocto and Buildroot issued releases with
  patched 4.8.2 compilers well before the (fixed) 4.8.3 release.  I think
  the most we can reasonably do without breaking some correctly-behaving
  toolchains is to emit a warning.
 
 Is it possible to compile a small code fragment and check the generated
 code for the bug?
 Possibly predicated on the broken version number to avoid false positives.

I don't see how - it looks like it requires an interrupt to occur at an
opportune moment to provoke the function to fail.  The alternative would
be to parse the assembly generated by the compiler to determine how it
is dealing with the stack.

I think the only viable solution here is that:

1. We blacklist the bad compiler versions outright in the kernel.
2. We /consider/ a testing a preprocessor symbol which when present
   indicates that these versions are fixed and should not be blacklisted.

The argument for (2) is that /if/ distros want to patch their compilers
to fix the problem, they /also/ have the ability to patch their compilers
to make them identifyable, and that is a far more reliable solution than
trying to parse the assembly output from multiple different GCC versions.

Remember, it's the distro's choice to fix these buggy compilers, so the
onus is on _them_ to deal with the mess they've created by doing so.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Fri, Oct 10, 2014 at 08:44:33PM -0500, Nathan Lynch wrote:
 On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
  We can blacklist these GCC versions quite easily.  We already have GCC
  3.3 blacklisted, and it's trivial to add others.  I would want to include
  some proper details about the bug, just like the other existing entries
  we already have in asm-offsets.c, where we name the functions that the
  compiler is known to break where appropriate.
 
 Before blacklisting anything, it's worth considering that simple version
 checks would break existing pre-4.8.3 compilers that have been patched
 for PR58854.  It looks like Yocto and Buildroot issued releases with
 patched 4.8.2 compilers well before the (fixed) 4.8.3 release.  I think
 the most we can reasonably do without breaking some correctly-behaving
 toolchains is to emit a warning.

I wish that it was possible to just do the warning thing, but unfortunately
evidence is that many people ignore compiler warnings, because they see
them appearing from the kernel soo often they have become de-sensitised
to them.

This is pretty obvious from the various nightly build systems which produce
the same warnings for months without any progress on them - some of them
can be quite serious (oops-able) where printf format strings are concerned.

  for some time that GCC 4.8.1 and GCC 4.8.2 _can_ lead to filesystem
  corruption, and have sat on their backsides doing nothing about getting
  it blacklisted for something like a year.
 
 Mea culpa, although I hadn't drawn the connection to FS corruption
 reports until now.  I have known about the issue for some time, but
 figured the prevalence of the fix in downstream projects largely
 mitigated the issue.

It's the FS corruption which swings it in favour of a #error - even if
we have a bunch of compilers around with that version which have the
problem fixed, it's /far/ better to #error out.  Those people who know
definitely that they have a fixed compiler can comment out the test
after checking that they do indeed have a fixed version, or are willing
to take the risk.

What we can't do is have kernels built by people who then run into FS
corruption because of this known issue.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Sat, Oct 11, 2014 at 11:54:32AM +0800, Peter Chen wrote:
 On Fri, Oct 10, 2014 at 08:44:33PM -0500, Nathan Lynch wrote:
  On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
   
   Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
   it seems that this has been known about for some time.)
  
  Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x  3
  are affected, as well as 4.9.0.
  
   We can blacklist these GCC versions quite easily.  We already have GCC
   3.3 blacklisted, and it's trivial to add others.  I would want to include
   some proper details about the bug, just like the other existing entries
   we already have in asm-offsets.c, where we name the functions that the
   compiler is known to break where appropriate.
  
  Before blacklisting anything, it's worth considering that simple version
  checks would break existing pre-4.8.3 compilers that have been patched
  for PR58854.  It looks like Yocto and Buildroot issued releases with
  patched 4.8.2 compilers well before the (fixed) 4.8.3 release.  I think
  the most we can reasonably do without breaking some correctly-behaving
  toolchains is to emit a warning.
 
 Yocto has PR58854 problem patch.
 
 http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/meta/recipes-devtools/gcc/gcc-4.8/0048-PR58854_fix_arm_apcs_epilogue.patch?h=daisy

Right, and we can provide links to these in the comments above the #error
so people have the right places to do a bit of research into whether their
compiler is safe.

It is unfortunate that they are indistinguishable from the broken versions,
but that's really a distro problem for causing that issue themselves -
especially given how serious this bug is.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Fri, Oct 10, 2014 at 12:47:06AM +0300, Aaro Koskinen wrote:
 Hi,
 
 On Thu, Oct 09, 2014 at 10:41:01PM +0200, Rabin Vincent wrote:
What GCC version are you using?

4.8.1 and 4.8.2 are known to miscompile the ARM kernel and these
find_get_entry() crashes with 0x involved smell a lot like the
earlier reports from kernels build with those compilers:

https://lkml.org/lkml/2014/6/25/456
https://lkml.org/lkml/2014/6/30/375
https://lkml.org/lkml/2014/6/30/660
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58854
https://lkml.org/lkml/2014/5/9/330
 
 Is it possible to blacklist those GCC versions on ARM somehow as it
 seems people are still using them?
 
 This bug also ruined a file system on one of my boxes last year
 (see e.g. http://marc.info/?l=linux-arm-kernelm=139033442527244w=2).

Given that, why the fsck (pun intended) did you not shout a little louder
about getting it blacklisted.  Looking at your marc.info URL, there's
very little information there which hints at filesystem corruption, and
it's a thread of only *one* message according to marc.info.

Even _if_ I did read the message you point to above, that on its own did
not hint at filesystem corruption.

So, would you please mind passing on further details about this,
specifically which function in the ext4 code is affected, so it can
be properly written up.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU bug with v3.17-rc3 ?

[Russell King - ARM Linux]

On Fri, Oct 10, 2014 at 08:57:43AM -0500, Felipe Balbi wrote:
 On Thu, Oct 09, 2014 at 04:07:15PM -0500, Felipe Balbi wrote:
  Hi,
  
  On Thu, Oct 09, 2014 at 03:46:37PM -0500, Felipe Balbi wrote:
   On Thu, Oct 09, 2014 at 10:41:01PM +0200, Rabin Vincent wrote:
On Thu, Oct 09, 2014 at 11:26:56AM -0500, Felipe Balbi wrote:
 alright, it's pretty deterministic however. Always on the same test, 
 no
 matter which USB controller, no matter if backing store is RAM or MMC.
 
 Those two undefined instructions on the disassembly caught my 
 attention,
 perhaps I'm facing a GCC bug ?

The undefined instructions are just ARM's BUG() implementation.

But did you see the question I asked you yesterday in your other thread?
http://www.spinics.net/lists/arm-kernel/msg368634.html
   
   hmm, completely missed that, sorry. I'm using 4.8.2, will try something
   else.
  
  seems to be working fine now, thanks. I'll leave test running overnight
  just in case.
 
 yup, ran over night without any problems.

Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
it seems that this has been known about for some time.)

We can blacklist these GCC versions quite easily.  We already have GCC
3.3 blacklisted, and it's trivial to add others.  I would want to include
some proper details about the bug, just like the other existing entries
we already have in asm-offsets.c, where we name the functions that the
compiler is known to break where appropriate.

However, I'm rather annoyed that there are people here who have known
for some time that GCC 4.8.1 and GCC 4.8.2 _can_ lead to filesystem
corruption, and have sat on their backsides doing nothing about getting
it blacklisted for something like a year.

When people talk about the ARM community being dysfunctional... well,
this kind of irresponsible behaviour just gives them more fodder to
throw at us.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 07/12] usb: chipidea: add a usb2 driver for ci13xxx

[Russell King - ARM Linux]

On Thu, Jul 17, 2014 at 07:19:15PM +0800, Peter Chen wrote:
 Currently, we are designing a generic driver, we don't know what's the
 hardware architecture, we are trying to find a solution how to set
 dma mask for all possible devices which will use this driver, Antoine's
 this patch is trying to cover this feature.
 
 For example, 
 
 Marvell Berlin doesn't need to set dma mask specific.
 http://marc.info/?l=linux-arm-kernelm=140205228507045w=2
 http://www.spinics.net/lists/linux-usb/msg110598.html

I can't make sense of those messages.

What are you saying - that ci_hdrc on Berlin does not use DMA at all?
Or that it doesn't care what the DMA mask is set to?

 Xilinx zynq needs to set dma mask as 0xFFF0
 http://marc.info/?l=linux-usbm=140384129921706w=2

Why?  The DMA mask does /not/ convey the DMA alignment requirements of
the transfers.  If you need it to be aligned to 16-bytes, then that's
something which is internal to the driver.  This is no different from
devices which require 32-bit alignment or 64-bit alignment.

You can't really expect the DMA subsystem to take care of that for you.
The DMA mask is about indicating what memory ranges are acceptable for
DMA, and not the alignment.

So, in this case, your DMA mask should be DMA_BIT_MASK(32), the same as
iMX.

However, if your driver does receive memory which is not appropriately
aligned, it is the driver's responsibility, not the DMA API, to deal
with that appropriately.

So, it sounds to me like:

- The DT code should be setting the DMA mask appropriately already.

- If the driver is using streaming DMA, should call:

err = dma_set_mask(pdev-dev, DMA_BIT_MASK(32));
if (err)
handle error;

- If the driver is using coherent DMA only:

err = dma_set_coherent_mask(pdev-dev, DMA_BIT_MASK(32));
if (err)
handle error;

- If the driver uses both coherent DMA and streaming DMA:

err = dma_set_mask_and_coherent(pdev-dev, DMA_BIT_MASK(32));
if (err)
handle error;

Where the _bus_ has restrictions on the available memory (iow, the
higher address bits), these should be dealt with inside the DMA API.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/18] usb: host: xhci-plat: Add clocks support

[Russell King - ARM Linux]

On Fri, Apr 25, 2014 at 04:07:00PM +0200, Gregory CLEMENT wrote:
 +#if defined(CONFIG_HAVE_CLK)
 +static int try_enable_clk(struct platform_device *pdev)
 +{
 + struct clk *clk = devm_clk_get(pdev-dev, NULL);
 +
 + /* Not all platforms have a clk so it is not an error if the clock
 +does not exists. */
 + if (!IS_ERR(clk))
 + if (clk_prepare_enable(clk))
 + return  -ENODEV;
 + return 0;
 +}
 +
 +static int try_disable_clk(struct platform_device *pdev)
 +{
 + struct clk *clk = devm_clk_get(pdev-dev, NULL);
 +
 + /* Not all platforms have a clk so it is not an error if the clock
 +does not exists. */
 + if (!IS_ERR(clk))
 + clk_disable_unprepare(clk);
 +
 + return 0;
 +}

OMG.

You do realise that clk_get() ref-counts against the module which
provided the clock, so this is akin to an explicit leaking module
ref-counts.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ARM: v3.13-rc1: USB regression

[Russell King - ARM Linux]

On Mon, Nov 25, 2013 at 06:33:02PM +0200, Aaro Koskinen wrote:
 Hi,
 
 On Sun, Nov 24, 2013 at 10:43:59PM +, Russell King - ARM Linux wrote:
  On Mon, Nov 25, 2013 at 12:22:47AM +0200, Aaro Koskinen wrote:
   [   33.967324] ohci ohci: Coherent DMA mask 0x (pfn 
   0xe-0xe) covers a smaller range of system memory than the DMA 
   zone pfn 0x0-0x10
   
   I bisected this to 4dcfa60071b3d23f0181f27d8519f12e37cefbb9 (ARM: DMA-API:
   better handing of DMA masks for coherent allocations). Reverting that
   commit makes the USB work again fine.
 
 [...]
 
  Better would be:
  
  #define __arch_dma_to_pfn(dev, addr)\
  ({ unsigned long pfn = (addr)  PAGE_SHIFT;\
 if (is_lbus_device(dev)) \
  pfn += PHYS_PFN_OFFSET -\
  (OMAP1510_LB_OFFSET  PAGE_SHIFT); \
 pfn; \
  })
  
  Can you try that in arch/arm/mach-omap1/include/mach/memory.h please?
 
 Still doesn't work:
 
 [   33.878790] ohci ohci: Coherent DMA mask 0x (pfn 
 0xfffe-0xe) covers a smaller range of system memory than the DMA zone 
 pfn 0x0-0x10
 [   33.894019] ohci ohci: can't setup: -12

Well, that looks technically better, rather unfortunate that we end up
going to negative PFNs though.

Without that change, could you try this instead please:

 arch/arm/mm/dma-mapping.c |9 +++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index f0ea0134e5a3..a18cfc53f445 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -9,6 +9,7 @@
  *
  *  DMA uncached mapping support.
  */
+#include linux/bootmem.h
 #include linux/module.h
 #include linux/mm.h
 #include linux/gfp.h
@@ -162,6 +163,8 @@ static u64 get_coherent_dma_mask(struct device *dev)
u64 mask = (u64)DMA_BIT_MASK(32);
 
if (dev) {
+   unsigned long max_dma_pfn;
+
mask = dev-coherent_dma_mask;
 
/*
@@ -173,6 +176,8 @@ static u64 get_coherent_dma_mask(struct device *dev)
return 0;
}
 
+   max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
+
/*
 * If the mask allows for more memory than we can address,
 * and we actually have that much memory, then fail the
@@ -180,7 +185,7 @@ static u64 get_coherent_dma_mask(struct device *dev)
 */
if (sizeof(mask) != sizeof(dma_addr_t) 
mask  (dma_addr_t)~0 
-   dma_to_pfn(dev, ~0)  arm_dma_pfn_limit) {
+   dma_to_pfn(dev, ~0)  max_dma_pfn) {
dev_warn(dev, Coherent DMA mask %#llx is larger than 
dma_addr_t allows\n,
 mask);
dev_warn(dev, Driver did not use or check the return 
value from dma_set_coherent_mask()?\n);
@@ -192,7 +197,7 @@ static u64 get_coherent_dma_mask(struct device *dev)
 * fits within the allowable addresses which we can
 * allocate.
 */
-   if (dma_to_pfn(dev, mask)  arm_dma_pfn_limit) {
+   if (dma_to_pfn(dev, mask)  max_dma_pfn) {
dev_warn(dev, Coherent DMA mask %#llx (pfn %#lx-%#lx) 
covers a smaller range of system memory than the DMA zone pfn 0x0-%#lx\n,
 mask,
 dma_to_pfn(dev, 0), dma_to_pfn(dev, mask) + 1,

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ARM: v3.13-rc1: USB regression

[Russell King - ARM Linux]

On Mon, Nov 25, 2013 at 12:22:47AM +0200, Aaro Koskinen wrote:
 Hi,
 
 With 3.13-rc1, the USB OHCI probe fails on Amstrad E3 (ARM/OMAP1)
 as follows:
 
 [   33.814705] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
 [   33.821482] ohci-omap: OHCI OMAP driver
 [   33.925153] ohci ohci: OMAP OHCI
 [   33.929087] ohci ohci: new USB bus registered, assigned bus number 1
 [   33.967324] ohci ohci: Coherent DMA mask 0x (pfn 0xe-0xe) 
 covers a smaller range of system memory than the DMA zone pfn 0x0-0x10
 [   33.982292] ohci ohci: can't setup: -12
 [   33.987898] ohci ohci: USB bus 1 deregistered
 [   33.992984] ohci: probe of ohci failed with error -12
 
 I bisected this to 4dcfa60071b3d23f0181f27d8519f12e37cefbb9 (ARM: DMA-API:
 better handing of DMA masks for coherent allocations). Reverting that
 commit makes the USB work again fine.

This is because of this:

#define __arch_dma_to_pfn(dev, addr)\
({ dma_addr_t __dma = addr; \
   if (is_lbus_device(dev)) \
__dma += PHYS_OFFSET - OMAP1510_LB_OFFSET;  \
   __phys_to_pfn(__dma);\
})

dma_addr_t is 32-bit.  PHYS_OFFSET - OMAP1510_LB_OFFSET is 0xe000.
Consider what the result of passing 0x as addr into this is.

Better would be:

#define __arch_dma_to_pfn(dev, addr)\
({ unsigned long pfn = (addr)  PAGE_SHIFT;\
   if (is_lbus_device(dev)) \
pfn += PHYS_PFN_OFFSET -\
(OMAP1510_LB_OFFSET  PAGE_SHIFT); \
   pfn; \
})

Can you try that in arch/arm/mach-omap1/include/mach/memory.h please?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: ohci-pxa27x: include linux/dma-mapping.h

[Russell King - ARM Linux]

On Fri, Nov 15, 2013 at 09:45:16AM +0100, Daniel Mack wrote:
 Include linux/dma-mapping.h to make the new functions available that are
 used since 22d9d8e83 (DMA-API: usb: use dma_set_coherent_mask()).
 
 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
 I got the following error while building for PXA platforms from Linus'
 current git head:
 
 drivers/usb/host/ohci-pxa27x.c: In function ‘ohci_pxa_of_init’:
 drivers/usb/host/ohci-pxa27x.c:310:2: error: implicit declaration of function 
 ‘dma_coerce_mask_and_coherent’ [-Werror=implicit-function-declaration]
 drivers/usb/host/ohci-pxa27x.c:310:2: error: implicit declaration of function 
 ‘DMA_BIT_MASK’ [-Werror=implicit-function-declaration]

Please put the errors (and warnings) in the commit description; they're
useful documentation for others who may encounter the same problem.

 diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
 index e89ac4d..3963834 100644
 --- a/drivers/usb/host/ohci-pxa27x.c
 +++ b/drivers/usb/host/ohci-pxa27x.c
 @@ -29,6 +29,7 @@
  #include linux/platform_data/usb-ohci-pxa27x.h
  #include linux/platform_data/usb-pxa3xx-ulpi.h
  #include linux/platform_device.h
 +#include linux/dma-mapping.h

Please review the #include statements and see if you can spot a broad
pattern there, and then decide whether your placement follows the
established pattern, thanks. :)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[Russell King - ARM Linux]

On Thu, Nov 14, 2013 at 12:09:46AM -0200, Fabio Estevam wrote:
 @@ -107,10 +108,22 @@ static int ci_hdrc_imx_probe(struct platform_device 
 *pdev)
   return ret;
   }
  
 + data-clk_phy = devm_clk_get(pdev-dev, phy);
 + if (IS_ERR(data-clk_phy)) {
 + data-clk_phy = NULL;
 + } else {

Please stop using NULL as a indicator with functions which only return
failure as an error pointer.  Replace the above three lines with

if (!IS_ERR(data-clk_phy)) {

 + ret = clk_prepare_enable(data-clk_phy);
 + if (ret) {
 + dev_err(pdev-dev,
 + Failed to enable clk_phy: %d\n, ret);
 + goto err_clk;
 + }
 + }
 +
   data-phy = devm_usb_get_phy_by_phandle(pdev-dev, fsl,usbphy, 0);
   if (IS_ERR(data-phy)) {
   ret = PTR_ERR(data-phy);
 - goto err_clk;
 + goto err_clk_phy;
   }
  
   pdata.phy = data-phy;
 @@ -157,6 +170,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
  
  disable_device:
   ci_hdrc_remove_device(data-ci_pdev);
 +err_clk_phy:
 + if (data-clk_phy)

This test should not be necessary if you've nested the error cleanup.

 + clk_disable_unprepare(data-clk_phy);
  err_clk:
   clk_disable_unprepare(data-clk);
   return ret;
 @@ -168,6 +184,8 @@ static int ci_hdrc_imx_remove(struct platform_device 
 *pdev)
  
   pm_runtime_disable(pdev-dev);
   ci_hdrc_remove_device(data-ci_pdev);
 + if (data-clk_phy)

if (!IS_ERR(data-clk_phy))

 + clk_disable_unprepare(data-clk_phy);
   clk_disable_unprepare(data-clk);
  
   return 0;
 -- 
 1.8.1.2
 
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 19/51] DMA-API: media: dt3155v4l: replace dma_set_mask()+dma_set_coherent_mask() with new helper

[Russell King - ARM Linux]

On Thu, Oct 31, 2013 at 09:46:40AM -0200, Mauro Carvalho Chehab wrote:
 Hi Russell,
 
 Em Mon, 30 Sep 2013 13:57:47 +0200
 Hans Verkuil hverk...@xs4all.nl escreveu:
 
  On 09/19/2013 11:44 PM, Russell King wrote:
   Replace the following sequence:
   
 dma_set_mask(dev, mask);
 dma_set_coherent_mask(dev, mask);
   
   with a call to the new helper dma_set_mask_and_coherent().
   
   Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
  
  Acked-by: Hans Verkuil hans.verk...@cisco.com
 
 Somehow, I lost your original post (I got unsubscribed on a few days 
 from all vger mailing lists at the end of september).
 
 I suspect that you want to sent this via your tree, right?

Yes please.

 If so:
 
 Acked-by: Mauro Carvalho Chehab m.che...@samsung.com

Added, thanks.

   - err = dma_set_mask(pdev-dev, DMA_BIT_MASK(32));
   - if (err)
   - return -ENODEV;
   - err = dma_set_coherent_mask(pdev-dev, DMA_BIT_MASK(32));
   + err = dma_set_mask_and_coherent(pdev-dev, DMA_BIT_MASK(32));
 if (err)
 return -ENODEV;

One thing I've just noticed is that return should be return err not
return -ENODEV - are you okay for me to change that in this patch?

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb/chipidea: fix oops on memory allocation failure

[Russell King - ARM Linux]

On Thu, Oct 17, 2013 at 01:50:17PM +0800, Peter Chen wrote:
 Below is my proposal fix for this problem: 
 
 diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
 index 42a0bd4..c1d05c4 100644
 --- a/drivers/usb/chipidea/host.c
 +++ b/drivers/usb/chipidea/host.c
 @@ -270,16 +270,18 @@ static void host_stop(struct ci_hdrc *ci)
  {
   struct usb_hcd *hcd = ci-hcd;
  
 - usb_remove_hcd(hcd);
 - usb_put_hcd(hcd);
 - if (ci-platdata-reg_vbus)
 - regulator_disable(ci-platdata-reg_vbus);
 + if (hcd) {
 + usb_remove_hcd(hcd);
 + usb_put_hcd(hcd);
 + if (ci-platdata-reg_vbus)
 + regulator_disable(ci-platdata-reg_vbus);
 + }
  }
  
  
  void ci_hdrc_host_destroy(struct ci_hdrc *ci)
  {
 - if (ci-role == CI_ROLE_HOST)
 + if (ci-role == CI_ROLE_HOST  ci-hcd)
   host_stop(ci);

If you're not calling host_stop() unless ci-hcd is setup, then you
don't need to check for that in host_stop() ?  Note that my oopsing
path is through the above function.

Anyway, Greg has already taken my patch from yesterday.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb/chipidea: fix oops on memory allocation failure

[Russell King - ARM Linux]

When CMA fails to initialize in v3.12-rc4, the chipidea driver oopses
the kernel while trying to remove and put the HCD which doesn't exist:

WARNING: CPU: 0 PID: 6 at /home/rmk/git/linux-rmk/arch/arm/mm/dma-mapping.c:511 
__dma_alloc+0x200/0x240()
coherent pool not initialised!
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Tainted: GW3.12.0-rc4+ #56
Workqueue: deferwq deferred_probe_work_func
Backtrace: 
[c001218c] (dump_backtrace+0x0/0x10c) from [c0012328] (show_stack+0x18/0x1c)
 r6:c05fd9cc r5:01ff r4: r3:df86ad00
[c0012310] (show_stack+0x0/0x1c) from [c05f3a4c] (dump_stack+0x70/0x8c)
[c05f39dc] (dump_stack+0x0/0x8c) from [c00230a8] 
(warn_slowpath_common+0x6c/0x8c)
 r4:df883a60 r3:df86ad00
[c002303c] (warn_slowpath_common+0x0/0x8c) from [c002316c] 
(warn_slowpath_fmt+0x38/0x40)
 r8: r7:1000 r6:c083b808 r5: r4:df2efe80
[c0023134] (warn_slowpath_fmt+0x0/0x40) from [c00196bc] 
(__dma_alloc+0x200/0x240)
 r3: r2:c05fda00
[c00194bc] (__dma_alloc+0x0/0x240) from [c001982c] (arm_dma_alloc+0x88/0xa0)
[c00197a4] (arm_dma_alloc+0x0/0xa0) from [c03e2904] (ehci_setup+0x1f4/0x438)
[c03e2710] (ehci_setup+0x0/0x438) from [c03cbd60] (usb_add_hcd+0x18c/0x664)
[c03cbbd4] (usb_add_hcd+0x0/0x664) from [c03e89f4] (host_start+0xf0/0x180)
[c03e8904] (host_start+0x0/0x180) from [c03e7c34] (ci_hdrc_probe+0x360/0x670
)
 r6:df2ef410 r5: r4:df2c3010 r3:c03e8904
[c03e78d4] (ci_hdrc_probe+0x0/0x670) from [c0311044] 
(platform_drv_probe+0x20/0x24)
[c0311024] (platform_drv_probe+0x0/0x24) from [c030fcac] 
(driver_probe_device+0x9c/0x234)
...
---[ end trace c88ccaf3969e8422 ]---
Unable to handle kernel NULL pointer dereference at virtual address 0028
pgd = c0004000
[0028] *pgd=
Internal error: Oops: 17 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Tainted: GW3.12.0-rc4+ #56
Workqueue: deferwq deferred_probe_work_func
task: df86ad00 ti: df882000 task.ti: df882000
PC is at usb_remove_hcd+0x10/0x150
LR is at host_stop+0x1c/0x3c
pc : [c03cacec]lr : [c03e88e4]psr: 6013
sp : df883b50  ip : df883b78  fp : df883b74
r10: c11f4c54  r9 : c0836450  r8 : df30c400
r7 : fff4  r6 : df2ef410  r5 :   r4 : df2c3010
r3 :   r2 :   r1 : df86b0a0  r0 : 
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c53c7d  Table: 2f29404a  DAC: 0015
Process kworker/u2:0 (pid: 6, stack limit = 0xdf882240)
Stack: (0xdf883b50 to 0xdf884000)
...
Backtrace: 
[c03cacdc] (usb_remove_hcd+0x0/0x150) from [c03e88e4] (host_stop+0x1c/0x3c)
 r6:df2ef410 r5: r4:df2c3010
[c03e88c8] (host_stop+0x0/0x3c) from [c03e8aa0] 
(ci_hdrc_host_destroy+0x1c/0x20)
 r5: r4:df2c3010
[c03e8a84] (ci_hdrc_host_destroy+0x0/0x20) from [c03e7c80] 
(ci_hdrc_probe+0x3ac/0x670)
[c03e78d4] (ci_hdrc_probe+0x0/0x670) from [c0311044] 
(platform_drv_probe+0x20/0x24)
[c0311024] (platform_drv_probe+0x0/0x24) from [c030fcac] 
(driver_probe_device+0x9c/0x234)
[c030fc10] (driver_probe_device+0x0/0x234) from [c030ff28] 
(__device_attach+0x44/0x48)
...
---[ end trace c88ccaf3969e8423 ]---

Fix this so at least we can continue booting and get to a shell prompt.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Tested-by: Russell King rmk+ker...@arm.linux.org.uk
---
 drivers/usb/chipidea/host.c |6 --
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 6f96795..64d7a6d 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -100,8 +100,10 @@ static void host_stop(struct ci_hdrc *ci)
 {
struct usb_hcd *hcd = ci-hcd;
 
-   usb_remove_hcd(hcd);
-   usb_put_hcd(hcd);
+   if (hcd) {
+   usb_remove_hcd(hcd);
+   usb_put_hcd(hcd);
+   }
if (ci-platdata-reg_vbus)
regulator_disable(ci-platdata-reg_vbus);
 }
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Tue, Oct 15, 2013 at 10:18:15AM +0800, Peter Chen wrote:
 So, the lessons for this topic are:
 
 - If one atomic variable's operation only includes one instruction like
 atomic_read and atomic_set, it is not meaningful for using atomic
 operation, we can just use bool instead of it.

The lesson here is that these are 100% equivalent as far as safety from
races is concerned:

a = atomic_read(v);a = v-counter;

atomic_set(v, b);  v-counter = b;

and in general, whenever atomic_read() gets used it's almost certainly
a sign of a bug.

Consider this (similar has been submitted):

a = atomic_read(v);
if (a != 0)
a += 1;

atomic_set(v, a);

and people have thought that somehow this is magically safe from races
because they're using atomic_t, and somehow that saves the universe.
The above is in fact no safer than:

a = *v;
if (a != 0)
a += 1;
*v = a;

The only thing that using atomic_* does is add a false sense of security
and a level of obfuscation to catch the unwary reviewer.

The reason is quite simple: a single access read in itself is atomic.
Either it has read the value, or it hasn't.  A single access store is
itself atomic.  Either the data has been written, or it hasn't.  The
issue is _always_ what you do around it.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Mon, Oct 14, 2013 at 03:55:48PM +0800, Peter Chen wrote:
 On Mon, Oct 14, 2013 at 10:04:58AM +0200, Lothar Waßmann wrote:
  Hi,
  
  Peter Chen wrote:
   This commit adds runtime and system power management support for
   chipidea core. The runtime pm support is controlled by glue
   layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
   
  [...]
   +#ifdef CONFIG_PM
   +static int ci_controller_suspend(struct device *dev)
   +{
   + struct ci_hdrc *ci = dev_get_drvdata(dev);
   +
   + dev_dbg(dev, at %s\n, __func__);
   +
   + if (atomic_read(ci-in_lpm))
   + return 0;
   +
  What does this 'atomic_read()' buy you over just testing/assinging a
  simple integer. Note that just because the function has 'atomic' in
  its name the sequence:
  atomic_read();
  ...
  atomic_set();
  does not magically become an atomic operation.
 
 I just want the read and set are atomic, not the operations
 between atomic_read and atomic_set.

There is nothing magical about atomic_read() or atomic_set():

#define atomic_read(v)  (*(volatile int *)(v)-counter)
#define atomic_set(v,i) (((v)-counter) = (i))

You might as well just read and write the variable directly if you're
going to do this.  The atomicity of atomic_t comes from the *other*
operations which can be done on an atomic_t, not from some magical
macro which starts with the name atomic_.

Lesson 1: whenever you see atomic_read() and atomic_set() in a patch
be very very very suspicious; it's 99% of times plainly wrong and a
sign of something being totally broken.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Mon, Oct 14, 2013 at 05:04:21PM +0800, Peter Chen wrote:
 It is for ARM, but for other platforms, it may not.

Wrong.  atomic_read() and atomic_set() are defined the same way and
have the same lack of atomicity across all architectures.  There is
nothing special about these over a standard load/store.  Your code
using this is quite simply broken and founded on false assumptions
about these macros.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Sat, Oct 12, 2013 at 05:35:03PM +0800, Peter Chen wrote:
 This commit adds runtime and system power management support for
 chipidea core. The runtime pm support is controlled by glue
 layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.

Let's look at the locking.

1. Runtime PM.  These callbacks are locked with a spinlock, which holds
   dev-power.lock.  This lock is taken either with or without disabling
   IRQs depending on whether runtime PM is IRQ safe or not.

2. Normal PM.  These callbacks are locked by holding dev-mutex.

Now, there's a little bit of protection between these two operations -
when normal PM places a device into a low power state, it 'gets' a
reference on the runtime PM to ensure no runtime PM transitions occur
while normal PM is active.  (See pm_runtime_get_noresume() in
device_prepare().)  This is only dropped when the normal PM resumes the
device.

Moreover, all runtime PM events are flushed before the suspend callback
occurs (see the pm_runtime_barrier() in __device_suspend()).

What that means is that you can't receive any runtime PM events while
you are in your suspend/resume callbacks.  So, each call is mutually
exclusive.

So, runtime PM callbacks vs normal PM callbacks for any single device
are all called with mutual exclusion - you won't have two running at
any time.

Hence, for the reasons stated previously about the non-atomic nature of
atomic_read()/atomic_set(), there's even more reasons that their use
here is just mere obfuscation: the accesses to this state tracking
variable is already guaranteed to be single-threaded by core code, so
the use of atomic_read()/atomic_set() just adds additional needless
confusion to this code.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/51] DMA mask changes

[Russell King - ARM Linux]

On Thu, Sep 26, 2013 at 10:23:08PM +0200, Rafał Miłecki wrote:
 2013/9/19 Russell King - ARM Linux li...@arm.linux.org.uk:
  This email is only being sent to the mailing lists in question, not to
  anyone personally.  The list of individuals is far to great to do that.
  I'm hoping no mailing lists reject the patches based on the number of
  recipients.
 
 Huh, I think it was enough to send only 3 patches to the b43-dev@. Like:
 [PATCH 01/51] DMA-API: provide a helper to set both DMA and coherent DMA masks
 [PATCH 14/51] DMA-API: net: b43: (...)
 [PATCH 15/51] DMA-API: net: b43legacy: (...)
 ;)
 
 I believe Joe has some nice script for doing it that way. When fixing
 some coding style / formatting, he sends only related patches to the
 given ML.

If I did that, then the mailing lists would not get the first patch,
because almost none of the lists would be referred to by patch 1.

Moreover, people complain when they don't have access to the full
patch series - they assume patches are missing for some reason, and
they ask for the rest of the series.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 36/51] DMA-API: usb: use dma_set_coherent_mask()

[Russell King - ARM Linux]

On Mon, Sep 23, 2013 at 02:27:39PM -0400, Alan Stern wrote:
 On Thu, 19 Sep 2013, Russell King wrote:
 
  The correct way for a driver to specify the coherent DMA mask is
  not to directly access the field in the struct device, but to use
  dma_set_coherent_mask().  Only arch and bus code should access this
  member directly.
  
  Convert all direct write accesses to using the correct API.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 
  diff --git a/drivers/usb/host/ehci-platform.c 
  b/drivers/usb/host/ehci-platform.c
  index f6b790c..5b0cd2d 100644
  --- a/drivers/usb/host/ehci-platform.c
  +++ b/drivers/usb/host/ehci-platform.c
 
  @@ -91,8 +91,9 @@ static int ehci_platform_probe(struct platform_device 
  *dev)
  dev-dev.platform_data = ehci_platform_defaults;
  if (!dev-dev.dma_mask)
  dev-dev.dma_mask = dev-dev.coherent_dma_mask;
  -   if (!dev-dev.coherent_dma_mask)
  -   dev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
  +   err = dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32));
  +   if (err)
  +   return err;
   
  pdata = dev_get_platdata(dev-dev);
 
 ehci-platform.c is a generic file, intended for use by multiple
 platforms.  Is it not possible that on some platforms, the arch or bus
 code may already have initialized the DMA masks?  Isn't something like 
 this (i.e., DMA bindings) planned for Device Tree?
 
 By eliminating the tests above and calling dma_set_coherent_mask or
 dma_coerce_mask_and_coherent unconditionally, this patch (and the next)
 would overwrite those initial settings.
 
 I don't know to what extent the same may be true for the other,
 platform-specific, drivers changed by this patch.  But it's something 
 to be aware of.

Please check the DMA API documentation:

=
For correct operation, you must interrogate the kernel in your device
probe routine to see if the DMA controller on the machine can properly
support the DMA addressing limitation your device has.  It is good
style to do this even if your device holds the default setting,
because this shows that you did think about these issues wrt. your
device.

The query is performed via a call to dma_set_mask():

int dma_set_mask(struct device *dev, u64 mask);

The query for consistent allocations is performed via a call to
dma_set_coherent_mask():

int dma_set_coherent_mask(struct device *dev, u64 mask);
=

So, All drivers which use DMA are supposed to issue the appropriate
calls to check the DMA masks before they perform DMA, even if the
default is correct.

And yes, this is all part of sorting out the DT mess - we should
start not from the current mess (which is really totally haphazard)
but from the well established position of how the DMA API _should_
be used.  What that means is that all drivers should be issuing
these calls as appropriate today.

The default mask setup when the device is created is just that -
it's a default mask, and it should not be used to decide anything
about the device.  That's something which the driver should compute
on its own accord and then inform the various other layers via the
appropriate call.

Remember, on PCI, even when we have 64-bit, we do not declare PCI
devices with a 64-bit DMA mask: that's up to PCI device drivers to
_try_ to set a 64-bit DMA mask, which when successful _allows_ them
to use 64-bit DMA.  If it fails, they have to only use 32-bit.  If
they want a smaller mask, the _driver_ has to set the smaller mask,
not the device creating code.

The reason we're into this (particularly on ARM) is that we got lazy
because we could get by with declaring a DMA mask at device creation
time, since all devices were statically declared.  Now it's time to
get rid of those old lazy ways and start doing things correctly and
to the requirements of the APIs.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 24/51] DMA-API: dma: pl330: add dma_set_mask_and_coherent() call

[Russell King - ARM Linux]

On Fri, Sep 20, 2013 at 07:26:27PM +0200, Heiko Stübner wrote:
 Am Donnerstag, 19. September 2013, 23:49:01 schrieb Russell King:
  The DMA API requires drivers to call the appropriate dma_set_mask()
  functions before doing any DMA mapping.  Add this required call to
  the AMBA PL08x driver.
   ^--- copy and paste error - should of course be PL330

Fixed, thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 42/51] DMA-API: usb: musb: use platform_device_register_full() to avoid directly messing with dma masks

[Russell King - ARM Linux]

On Fri, Sep 20, 2013 at 08:11:25AM -0500, Felipe Balbi wrote:
 Hi,
 
 On Fri, Sep 20, 2013 at 12:14:38AM +0100, Russell King wrote:
  Use platform_device_register_full() for those drivers which can, to
  avoid messing directly with DMA masks.  This can only be done when
  the driver does not need to access the allocated musb platform device
  from within its callbacks, which may be called during the musb
  device probing.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 
 you want me to carry this one through my tree or you prefer getting my
 Acked-by ? Either way works for me:
 
 Acked-by: Felipe Balbi ba...@ti.com
 
 there's also the third option of me setting up a branch with only this
 patch and we both merge it, that'd also work.

I think this patch is sufficiently stand-alone that it should be fine
if you want to take it through your tree.  That may be better in the
long run to avoid conflicts with this patch and any future work in
this area during this cycle.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 39/51] DMA-API: others: use dma_set_coherent_mask()

[Russell King - ARM Linux]

On Fri, Sep 20, 2013 at 07:16:52AM -0500, Tejun Heo wrote:
 On Fri, Sep 20, 2013 at 12:11:38AM +0100, Russell King wrote:
  The correct way for a driver to specify the coherent DMA mask is
  not to directly access the field in the struct device, but to use
  dma_set_coherent_mask().  Only arch and bus code should access this
  member directly.
  
  Convert all direct write accesses to using the correct API.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 
 Acked-by: Tejun Heo t...@kernel.org
 
 The patch is pretty widely spread.  I don't mind how it gets routed
 but what's the plan?

The plan is... I'm going to try and avoid going through the hell of
re-posting this patch series to all the recipients another time...
(It's taken some 17 hours and lots of hand holding to get this patch
set out without exim jumping off a cliff into deep OOM - soo deep that
even the OOM killer doesn't run and the CPU is 100% idle because
every single process stuck in an uninterruptible sleep waiting for
every other process to free some memory - ouch!)

I know that dealing with this patch set will be a problem due to how
widespread this is, but much of the driver level changes come down to
depending on a couple of patches.  One solution would be if I published
a branch with just the dependencies in, which subsystem maintainers
could pull, and then apply the appropriate patches on top.

Another would be if subsystem maintainers are happy that I carry them,
I can add the acks, and then later on towards the end of the cycle,
provide a branch subsystem maintainers could pull.

Or... if you can think of something easier...
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/51] DMA-API: provide a helper to set both DMA and coherent DMA masks

[Russell King - ARM Linux]

On Fri, Sep 20, 2013 at 02:21:37AM +0100, Ben Hutchings wrote:
 On Thu, 2013-09-19 at 22:25 +0100, Russell King wrote:
 [...]
  -dma_set_coherent_mask() will always be able to set the same or a
  -smaller mask as dma_set_mask(). However for the rare case that a
  +The coherent coherent mask will always be able to set the same or a
  +smaller mask as the streaming mask. However for the rare case that a
 [...]
 
 The new wording doesn't make sense; a mask doesn't set itself.  I would
 suggest:
 
 The coherent mask can always be set to the same or a smaller mask than
 the streaming mask.

Yes, the original sentence is not particularly good, but I think even
your modified version can be interpreted as a mask setting itself
for all the same reasons that the original can be (which mask does same
refer to?)

Even so, I prefer your version.  Thanks. :)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/51] DMA mask changes

[Russell King - ARM Linux]

This started out as a request to look at the DMA mask situation, and how
to solve the issues which we have on ARM - notably how the DMA mask
should be setup.

However, I started off reviewing how the dma_mask and coherent_dma_mask
was being used, and what I found was rather messy, and in some cases
rather buggy.  I tried to get some of the bug fixes in before the last
merge window, but it seems that the maintainers preferred to have the
full solution rather than a simple -rc suitable bug fix.

So, this is an attempt to clean things up.

The first point here is that drivers performing DMA should be calling
dma_set_mask()/dma_set_coherent_mask() in their probe function to verify
that DMA can be performed.  Lots of ARM drivers omit this step; please
refer to the DMA API documentation on this subject.

What this means is that the DMA mask provided by bus code is a default
value - nothing more.  It doesn't have to accurately reflect what the
device is actually capable of.  Apart from the storage for dev-dma_mask
being initialised for any device which is DMA capable, there is no other
initialisation which is strictly necessary at device creation time.

Now, these cleanups address two major areas:
1. The setting of DMA masks, particularly when both the coherent and
   streaming DMA masks are set together.

2. The initialisation of DMA masks by drivers - this seems to be becoming
   a popular habbit, one which may not be entirely the right solution.
   Rather than having this scattered throughout the tree, I've pulled
   that into a central location (and called it coercing the DMA mask -
   because it really is about forcing the DMA mask to be that value.)

3. Finally, addressing the long held misbelief that DMA masks somehow
   correspond with physical addresses.  We already have established
   long ago that dma_addr_t values returned from the DMA API are the
   values which you program into the DMA controller, and so are the
   bus addresses.  It is _only_ sane that DMA masks are also bus
   related too, and not related to physical address spaces.

(3) is a very important point for LPAE systems, which may still have
less than 4GB of memory, but this memory is all located above the 4GB
physical boundary.  This means with the current model, any device
using a 32-bit DMA mask fails - even though the DMA controller is
still only a 32-bit DMA controller but the 32-bit bus addresses map
to system memory.  To put it another way, the bus addresses have a
4GB physical offset on them.

This email is only being sent to the mailing lists in question, not to
anyone personally.  The list of individuals is far to great to do that.
I'm hoping no mailing lists reject the patches based on the number of
recipients.

Patches based on v3.12-rc1.

 Documentation/DMA-API-HOWTO.txt   |   37 +--
 Documentation/DMA-API.txt |8 +++
 arch/arm/include/asm/dma-mapping.h|8 +++
 arch/arm/mm/dma-mapping.c |   49 ++--
 arch/arm/mm/init.c|   12 +++---
 arch/arm/mm/mm.h  |2 +
 arch/powerpc/kernel/vio.c |3 +-
 block/blk-settings.c  |8 ++--
 drivers/amba/bus.c|6 +--
 drivers/ata/pata_ixp4xx_cf.c  |5 ++-
 drivers/ata/pata_octeon_cf.c  |5 +-
 drivers/block/nvme-core.c |   10 ++---
 drivers/crypto/ixp4xx_crypto.c|   48 ++--
 drivers/dma/amba-pl08x.c  |5 ++
 drivers/dma/dw/platform.c |8 +--
 drivers/dma/edma.c|6 +--
 drivers/dma/pl330.c   |4 ++
 drivers/firmware/dcdbas.c |   23 +-
 drivers/firmware/google/gsmi.c|   13 +++--
 drivers/gpu/drm/exynos/exynos_drm_drv.c   |6 ++-
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c  |5 +-
 drivers/media/platform/omap3isp/isp.c |6 +-
 drivers/media/platform/omap3isp/isp.h |3 -
 drivers/mmc/card/queue.c  |3 +-
 drivers/mmc/host/sdhci-acpi.c |5 +-
 drivers/net/ethernet/broadcom/b44.c   |3 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c  |8 +---
 drivers/net/ethernet/brocade/bna/bnad.c   |   13 ++
 drivers/net/ethernet/emulex/benet/be_main.c   |   12 +
 drivers/net/ethernet/intel/e1000/e1000_main.c |9 +---
 drivers/net/ethernet/intel/e1000e/netdev.c|   18 +++-
 drivers/net/ethernet/intel/igb/igb_main.c |   18 +++-
 drivers/net/ethernet/intel/igbvf/netdev.c |   18 +++-
 drivers/net/ethernet/intel/ixgb/ixgb_main.c   |   16 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 

Re: [PATCH] USB: set device dma_mask without reference to global data

[Russell King - ARM Linux]

On Wed, May 08, 2013 at 01:42:11AM +0200, Arnd Bergmann wrote:
 On Wednesday 08 May 2013, Greg Kroah-Hartman wrote:
  On Tue, May 07, 2013 at 04:53:52PM -0600, Stephen Warren wrote:
   From: Stephen Warren swar...@nvidia.com
 
   Suggested-by: Arnd Bergmann a...@arndb.de
   Signed-off-by: Stephen Warren swar...@nvidia.com
  
  So this needs to go in for 3.10, right?  Any older kernels as well?  If
  so, which ones?
 
 The fix should definitely go into 3.10, but I'd suggest waiting with
 the backport for a couple of -rc releases to avoid possible regressions.
 We know that the current code is broken, but few people fully understand
 what is going on with coherent_dma_mask, so it might cause new problems
 in combination with some other unknown bug, and I don't see this as
 urgent: none of the ARM defconfigs build this driver as a loadable
 module and there is no bug in the built-in case. For some reason, only
 the ARM back-end drivers are broken.
 
 The first occurence was apparently in 3.3, but only in ehci-tegra.c,
 while the other drivers subsequently copied the bug.

I've already suggested this approach:

diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index dc662fc..51bb740 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 struct omap_device;
 
 struct pdev_archdata {
+   u64 dma_mask;
 #ifdef CONFIG_ARCH_OMAP
struct omap_device *od;
 #endif

And then we can have dev-dma_mask pointed at that instead, which fully
eliminates any possible problems of things like dma_set_mask() interfering
with dma_set_coherent_mask().

Even better - because this is a common problem - would be to make 'dma_mask'
be a member of struct platform_device so that all arches can sort this
out once and for all (correction: generic code/drivers can in an arch-
independent way.)  IOW:

diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 9abf1db..121c74c 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -26,6 +26,7 @@ struct platform_device {
struct device   dev;
u32 num_resources;
struct resource *resource;
+   u64 dma_mask;
 
const struct platform_device_id *id_entry;
 

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: set device dma_mask without reference to global data

[Russell King - ARM Linux]

On Wed, May 08, 2013 at 09:24:44AM +0200, Arnd Bergmann wrote:
 It probably should. The main thing is that the dma_mask setting in
 of_platform (and elsewhere) is a mess and that nobody so far had the
 guts to try to get it right for good.
 
 Setting a 32 bit DMA mask is /probably/ the right default on all
 ARM systems, but there are caveats:
 
 - Once you get to systems with larger than 32 bit addressing (powerpc64,
   arm64, arm32 with LPAE), it's not so obvious: you may have some devices
   that need a 32 bit mask and some that need a 64 bit mask.

This is precisely why drivers should be using dma_set_mask() and the
coherent version to provide the mask for which the driver knows about
- iow, if the device itself is only capable of 32-bit DMA, then the
device must use dma_set_mask(dev, DMA_BIT_MASK(32)).
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
 Hi,
 
 On Fri, May 03, 2013 at 12:13:53AM +0100, Russell King - ARM Linux wrote:
  Signed-off-by: Alexander Shiyan shc_w...@mail.ru
  ---
   arch/arm/mach-omap2/usb-host.c | 21 +
   1 file changed, 17 insertions(+), 4 deletions(-)
  
  diff --git a/arch/arm/mach-omap2/usb-host.c 
  b/arch/arm/mach-omap2/usb-host.c
  index aa27d7f..8d17a0d 100644
  --- a/arch/arm/mach-omap2/usb-host.c
  +++ b/arch/arm/mach-omap2/usb-host.c
  @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, 
  char *dev_id, char *dev_supply,
  supplies-dev_name = dev_id;
   
  reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
  -   if (!reg_data)
  +   if (!reg_data) {
  +   kfree(supplies);
  return -ENOMEM;
  +   }
   
  reg_data-constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
  reg_data-consumer_supplies = supplies;
  @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, 
  char *dev_id, char *dev_supply,
   
  config = kmemdup(hsusb_reg_config, sizeof(hsusb_reg_config),
  GFP_KERNEL);
  -   if (!config)
  +   if (!config) {
  +   kfree(supplies);
  +   kfree(reg_data);
  return -ENOMEM;
  +   }
   
  config-supply_name = name;
  config-gpio = gpio;
  @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, 
  char *dev_id, char *dev_supply,
   
  /* create a regulator device */
  pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
  -   if (!pdev)
  +   if (!pdev) {
  +   kfree(supplies);
  +   kfree(reg_data);
  +   kfree(config);
  return -ENOMEM;
  +   }
   
  pdev-id = PLATFORM_DEVID_AUTO;
  pdev-name = reg_name;
  pdev-dev.platform_data = config;
   
  ret = platform_device_register(pdev);
  -   if (ret)
  +   if (ret) {
  pr_err(%s: Failed registering regulator %s for %s\n,
  __func__, name, dev_id);
  +   kfree(supplies);
  +   kfree(reg_data);
  +   kfree(config);
  +   }
 
 Might be better to switch to devm_XXX managed functions?
   
   I don't think it makes sense since the platform_device hasn't been
   registered yet.
   
   Still, patch can be improved with proper goto labels instead of
   sprinkling different kfree() calls in every single error branch.
   
If anyone can rewrite driver to use devm_xx, it would have been better.
I'm not going to redo the patch yet, let it be so, I just showed a point
for OMAP-developers.
   
   fair enough.
  
  Well, as long as this crap violates the driver model by using kfree() on
  a device...  Devices are refcounted and must only be freed when the
  refcount drops to zero.
 
 read the patch again, there's no kfree() on any device. There is a kfree
 of supplies, reg_data and config.
 
 On top of that, the code being changed here doesn't even exist, so I
 wonder which tree is this code based off. usb-host.c has always being
 using omap_device_build() which internally calls
 platform_device_alloc().
 
  This is exactly why we have platform_device_alloc(),
  platform_device_register_full() and friends - so that people don't have to
  fsck around with kzalloc themselves and get it wrong like the above does.
  
  Would you like me to pass your details to gregkh for another one of his
  public humilation exercises over basic kernel programming stuff? :)
 
 How about we pass yours for not reading the patch before flaming ? Note
 that $SUBJECT is *not* touching at all that line which kzallocs a
 platform_device. Wrong as it is, it's not part of $SUBJECT.

It's really simple.  You do not use k*alloc with platform devices.  And
you reject any patch which contains that, and point it out to the patch
author.

It really doesn't matter if there's a kfree or not.  The fact is you do
not allow it in any situation, because such bad practises get copied
and then you end up with kfree's.

How about you gain an understanding of this stuff and why this stuff is
soo hot.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
 How about we pass yours for not reading the patch before flaming ?

And another thing.  If you want to be flamed, continue with that tone.
The problem here is that *you* have not been flamed in the previous
message.  The previous message was all about pointing out an error in
the code in a way that hopefully you will take notice of, because you
showed no sign of taking any notice of my previous mail.

If you want to be a total dork, continue behaving as you currently are.
There, *now* I *have* flamed you!
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 11:30:09AM +0300, Felipe Balbi wrote:
 Hi,
 
 On Fri, May 03, 2013 at 09:15:10AM +0100, Russell King - ARM Linux wrote:
This is exactly why we have platform_device_alloc(),
platform_device_register_full() and friends - so that people don't have 
to
fsck around with kzalloc themselves and get it wrong like the above 
does.

Would you like me to pass your details to gregkh for another one of his
public humilation exercises over basic kernel programming stuff? :)
   
   How about we pass yours for not reading the patch before flaming ? Note
   that $SUBJECT is *not* touching at all that line which kzallocs a
   platform_device. Wrong as it is, it's not part of $SUBJECT.
  
  It's really simple.  You do not use k*alloc with platform devices.  And
 
 agree, no discussions here
 
  you reject any patch which contains that, and point it out to the patch
  author.
  
  It really doesn't matter if there's a kfree or not.  The fact is you do
  not allow it in any situation, because such bad practises get copied
  and then you end up with kfree's.
  
  How about you gain an understanding of this stuff and why this stuff is
  soo hot.
 
 how about you look at the git log to figure out I had nothing to do with
 that original patch which added k*alloc to the pdev ?

How about you realise I haven't got time to fuck around like that at the
moment, so in the interests of getting people to fix the glaring error
I commented on it instead in a relevant thread in the hope that some
prat like you would take notice and ask for it to be fixed by someone
working on the driver.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 11:39:00AM +0300, Felipe Balbi wrote:
 Hi,
 
 On Fri, May 03, 2013 at 09:32:06AM +0100, Russell King - ARM Linux wrote:
  On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
   How about we pass yours for not reading the patch before flaming ?
  
  And another thing.  If you want to be flamed, continue with that tone.
  The problem here is that *you* have not been flamed in the previous
  message.  The previous message was all about pointing out an error in
  the code in a way that hopefully you will take notice of, because you
  showed no sign of taking any notice of my previous mail.
  
  If you want to be a total dork, continue behaving as you currently are.
  There, *now* I *have* flamed you!
 
 what a load of crap. Just because it wasn't pointed out in my previous
 email doesn't automatically mean I haven't noticed. Anyway, I won't be
 doing this all day.

Yes you are a load of crap *you* spout about people flaming you.  Well, if
you can't be bothered to acknowledge a problem then you can expect to get
resends because it's not clear that the message has been received.  And
you do *not* call resends flames back to those who were kind enough put
the effort in to tell you about them more than once.

That's just down right insulting and flame provoking.  Which is exactly
how we ended up here.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Thu, May 02, 2013 at 07:40:58PM +0300, Felipe Balbi wrote:
 Hi,
 
 On Thu, May 02, 2013 at 08:28:44PM +0400, Alexander Shiyan wrote:
   On 20:03-20130502, Alexander Shiyan wrote:

Signed-off-by: Alexander Shiyan shc_w...@mail.ru
---
 arch/arm/mach-omap2/usb-host.c | 21 +
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/usb-host.c 
b/arch/arm/mach-omap2/usb-host.c
index aa27d7f..8d17a0d 100644
--- a/arch/arm/mach-omap2/usb-host.c
+++ b/arch/arm/mach-omap2/usb-host.c
@@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char 
*dev_id, char *dev_supply,
supplies-dev_name = dev_id;
 
reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
-   if (!reg_data)
+   if (!reg_data) {
+   kfree(supplies);
return -ENOMEM;
+   }
 
reg_data-constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
reg_data-consumer_supplies = supplies;
@@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char 
*dev_id, char *dev_supply,
 
config = kmemdup(hsusb_reg_config, sizeof(hsusb_reg_config),
GFP_KERNEL);
-   if (!config)
+   if (!config) {
+   kfree(supplies);
+   kfree(reg_data);
return -ENOMEM;
+   }
 
config-supply_name = name;
config-gpio = gpio;
@@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char 
*dev_id, char *dev_supply,
 
/* create a regulator device */
pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
-   if (!pdev)
+   if (!pdev) {
+   kfree(supplies);
+   kfree(reg_data);
+   kfree(config);
return -ENOMEM;
+   }
 
pdev-id = PLATFORM_DEVID_AUTO;
pdev-name = reg_name;
pdev-dev.platform_data = config;
 
ret = platform_device_register(pdev);
-   if (ret)
+   if (ret) {
pr_err(%s: Failed registering regulator %s for %s\n,
__func__, name, dev_id);
+   kfree(supplies);
+   kfree(reg_data);
+   kfree(config);
+   }
   
   Might be better to switch to devm_XXX managed functions?
 
 I don't think it makes sense since the platform_device hasn't been
 registered yet.
 
 Still, patch can be improved with proper goto labels instead of
 sprinkling different kfree() calls in every single error branch.
 
  If anyone can rewrite driver to use devm_xx, it would have been better.
  I'm not going to redo the patch yet, let it be so, I just showed a point
  for OMAP-developers.
 
 fair enough.

Well, as long as this crap violates the driver model by using kfree() on
a device...  Devices are refcounted and must only be freed when the
refcount drops to zero.

This is exactly why we have platform_device_alloc(),
platform_device_register_full() and friends - so that people don't have to
fsck around with kzalloc themselves and get it wrong like the above does.

Would you like me to pass your details to gregkh for another one of his
public humilation exercises over basic kernel programming stuff? :)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V9 PATCH 01/12] usb: phy: protect phy init and shutdown for mutiple deivces

[Russell King - ARM Linux]

On Mon, Apr 29, 2013 at 09:24:41PM +0300, Felipe Balbi wrote:
 On Wed, Apr 24, 2013 at 02:23:15AM -0400, Chao Xie wrote:
  diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
  index 6b5978f..98d7e60 100644
  --- a/include/linux/usb/phy.h
  +++ b/include/linux/usb/phy.h
  @@ -87,6 +87,14 @@ struct usb_phy {
  /* to support controllers that have multiple transceivers */
  struct list_headhead;
   
  +   /*
  +* PHY may be shared by multiple devices.
  +* mutex and refcount are used to make sure PHY only initialize or
  +* shutdown once.
 
 bad grammar in this sentence.
 
  +*/
  +   struct mutexphy_mutex;
  +   unsigned intrefcount;
 
 why don't you use an atomic_t ?

Possibly because of this:

+   mutex_lock(x-phy_mutex);  
+   if (x-refcount++ == 0  x-init)  
+   ret = x-init(x);   
+   mutex_unlock(x-phy_mutex);

This code structure has the effect that with two concurrent callers, one
will be blocked while the other calls the init function, and both will
not pass until the init function has completed.

Using an atomic type does not provide that guarantee.  Consider:

if (atomic_inc_return(x-atomic) == 1  x-init)
ret = x-init(x);

when two concurrent callers occur.  Or even consider one caller to the
shutdown function and another which comes into the init path while the
shutdown function is still running.

Atomic types are all well and good but they are also horrendously
dangerous when abused in ways like you're suggesting.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/9] Reorganize R8A7779/Marzen USB code

[Russell King - ARM Linux]

On Sat, Apr 20, 2013 at 01:55:12AM +0400, Sergei Shtylyov wrote:
 Hello.
 
Here's the set of 9 patches against the Simon Horman's 'renesas.git' repo,
 'renesas-next-20130419' tag.  It was created to fix the shortcomings in the
 R8A7779/Marzen USB platform code and R8A7779 USB common PHY driver, and so
 spans both arch/arm/mach-shmobile/ and drivers/usb/ subtrees (some patches 
 have
 to touch both subtrees). The patches were conceived with the complete
 bisectability goal in mind.
 
 [1/9] ARM: shmobile: Marzen: move USB EHCI, OHCI, and PHY devices to R8A7779 
 code
 [2/9] ehci-platform: add pre_setup() method to platform data
 [3/9] ARM: shmobile: r8a7779: setup EHCI internal buffer
 [4/9] rcar-phy: remove EHCI internal buffer setup
 [5/9] ARM: shmobile: r8a7779: remove USB PHY 2nd memory resource
 [6/9] rcar-phy: correct base address
 [7/9] rcar-phy: add platform data
 [8/9] ARM: shmobile: Marzen: pass platform data to USB PHY device
 [9/9] rcar-phy: handle platform data

Please can you rethink how you use get_maintainers.pl and send out
patches?

Sending them To everyone who get_maintainers.pl spits out is all well
and good, but it means that there's no discrimination between those who
have a primary interest in the patches and those who don't.

Examples of people with a primary interest: those maintaining the
platforms and/or those who you want to merge your patches.  These
should be on the To: line.

Those who don't: everyone else who may have an interest - these go on
the Cc: line.

You're about the only person I regularly get patches for platforms which
are sent To: me all the time, and really they are not important for
me (though they are important enough for me to be Cc'd).

What I'm saying is that having come back from a vacation last Tuesday,
I could not just scan through this mailbox for all messages marked To
me and treat those as being more important than everything else, because
soo many of yours were captured by that.  And I'm still tacking the
backlog today, almost a week later.

Please be more considerate to those who you're mailing.

(Also, make sure you don't honor the Mail-Followup-To: header, which
will lead your mailer to keep placing all recipients into the To:
header on reply - because it breaks the above reasoning.  The To:
header should be those recipients who you are directly discussing, and
Cc: is for interested side parties.)

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V8 PATCH 01/16] usb: phy: mv_usb2: add PHY driver for marvell usb2 controller

[Russell King - ARM Linux]

On Tue, Mar 05, 2013 at 10:03:01AM +0800, Chao Xie wrote:
 On Mon, Mar 4, 2013 at 10:21 PM, Felipe Balbi ba...@ti.com wrote:
  On Wed, Feb 20, 2013 at 11:07:11PM -0500, Chao Xie wrote:
  + for (i = 0; i  mv_phy-clks_num; i++) {
  + mv_phy-clks[i] = devm_clk_get(pdev-dev,
  + pdata-clkname[i]);
 
  *NEVER* pass clock names via platform_data, this is utterly wrong.
 
 without device tree support, the only way we can get the clock is the pdata.
 the use phy have mutiple clocks.
 So what do you suggest to handle it?

Then you don't understand the clk API at all.

Read the documentation in include/linux/clk.h for clk_get().

The first parameter is the device which you're interested in getting the
clock for.

The second parameter defines the INPUT as a string to THAT DEVICE.  It
is specific to the device.  It is NOT the system name of the clock.

So, if you have a function clock and an interface clock to a device,
then use a name like fck for the function clock and ick for the
interface clock.

Do _NOT_ make the mistake of using global clock names.  People have done
that many times in the past and got into horrid sticky problems - and
ended up with _far_ more code than is really necessary if you do things
the right way.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V8 PATCH 01/16] usb: phy: mv_usb2: add PHY driver for marvell usb2 controller

[Russell King - ARM Linux]

On Wed, Mar 06, 2013 at 04:24:58PM +0800, Chao Xie wrote:
 The clock numbers and names are depent of SOCes,

No they aren't.  The clock names used to describe them in your documentation
may vary, but their _purpose_ for the sake of the device will be fixed -
and you should name them appropriately from the _device_ point of view.

Not the SoC point of view.  That way leads to total madness.  We've
proven this over the years that we've had the clk API and people have
come up with trash implementations that do that crap.  After many
years of struggling, they've seen the light and fixed their shite up
to work the way I originally intended, and... instantly benefited from
it.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 9/9] usb: chipidea: udc: fix the oops when plugs in usb cable after rmmod gadget

[Russell King - ARM Linux]

On Wed, Mar 06, 2013 at 05:56:40PM +0800, Peter Chen wrote:
 root@freescale ~$ ci_hdrc ci_hdrc.0: Connected to host
 Unable to handle kernel paging request at virtual address 7f01171c
 pgd = 80004000
 [7f01171c] *pgd=4fa1e811, *pte=, *ppte=
 Internal error: Oops: 7 [#1] SMP ARM
 Modules linked in: f_acm libcomposite u_serial [last unloaded: g_serial]
 CPU: 0Not tainted  (3.8.0-rc5+ #221)
   PC is at _gadget_stop_activity+0xbc/0x128
   LR is at ep_fifo_flush+0x68/0xa0


Why much of the oops dump indented?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] ARM: OMAP2: MUSB: Specify omap4 has mailbox

[Russell King - ARM Linux]

On Wed, Feb 06, 2013 at 01:22:31PM +0200, Felipe Balbi wrote:
 there's a little more to it. When running allmodconfig,
 CONFIG_ARCH_MULTIPLATFORM is set but none of the other ARCHes
 (ARCH_OMAP, ARCH_AT91, ARCH_VERSATILE, etc) are set, so it turned out
 that the driver wasn't even included on my build test.
 
 Russell, is this expected (the MULTIPLATFORM thing) ? Just so I fix my
 build test scripts to use another defconfig instead of allmod and
 allyes.

Don't know.  Most of that logic is sitting in arm-soc.  Nothing obvious
stands out as to why that's happening.

The first thing to check is which of the CONFIG_ARCH_MULTI_* options are
set.  It should be ARCH_MULTI_V7 and ARCH_MULTI_V6_V7.  This should then
allow ARCH_OMAP2PLUS to be set.

However, I've said this before, and I'll say it again: if you want to use
the all* targets, it's better to seed the configuration so that you end
up with the platform you're targetting selected.  I've always done this
with the build system, and even before it when I've wanted to use the all*
targets, because generally I want that behaviour, but I want it for a
particular platform.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/30] USB: ehci-omap: Use devm_request_and_ioremap()

[Russell King - ARM Linux]

On Tue, Jan 29, 2013 at 10:34:53AM -0500, Alan Stern wrote:
 On Mon, 28 Jan 2013, Russell King - ARM Linux wrote:
 
  On Mon, Jan 28, 2013 at 10:17:33AM -0500, Alan Stern wrote:
   On Mon, 28 Jan 2013, Roger Quadros wrote:
   
Make use of devm_request_and_ioremap() and correct comment.
   
   Didn't a big patch come through recently converting all usages of 
   devm_request_and_ioremap() to another function (I forget the name) that 
   does its own error reporting and returns an ERR_PTR value?
   
   (Checks the mailing list archive...)  Ah, here it is:
   
 http://marc.info/?l=linux-usbm=135876311801537w=2
   
   And the new function is devm_ioremap_resource().  We should avoid 
   adding new usages of an old interface.
  
  Maybe... if devm_ioremap_resource() was already in the kernel.  The
  problem at the moment is it isn't, and it'll probably be rather
  horrid for everyone to deal with especially when it comes to testing.
 
 Not in the kernel yet?  I didn't realize that.  Looks like Thierry 
 Reding will have some clean-up work to do when it does get in.

It may be in linux-next, but it isn't in Linus' yet... (of course, it's
probably scheduled for the upcoming merge window).  Certainly:

$ grep -r devm_ioremap_resource include

against Linus' tree (6abb7c2) still turns up nothing.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/30] USB: ehci-omap: Use devm_request_and_ioremap()

[Russell King - ARM Linux]

On Mon, Jan 28, 2013 at 10:17:33AM -0500, Alan Stern wrote:
 On Mon, 28 Jan 2013, Roger Quadros wrote:
 
  Make use of devm_request_and_ioremap() and correct comment.
 
 Didn't a big patch come through recently converting all usages of 
 devm_request_and_ioremap() to another function (I forget the name) that 
 does its own error reporting and returns an ERR_PTR value?
 
 (Checks the mailing list archive...)  Ah, here it is:
 
   http://marc.info/?l=linux-usbm=135876311801537w=2
 
 And the new function is devm_ioremap_resource().  We should avoid 
 adding new usages of an old interface.

Maybe... if devm_ioremap_resource() was already in the kernel.  The
problem at the moment is it isn't, and it'll probably be rather
horrid for everyone to deal with especially when it comes to testing.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V4 PATCH 18/26] usb: phy: mv_usb2_phy: add externel chip support

[Russell King - ARM Linux]

On Mon, Jan 21, 2013 at 05:07:36AM -0500, Chao Xie wrote:
 + mv_phy-extern_chip.head = devm_kzalloc(pdev-dev,
 + sizeof(*mv_phy-extern_chip.head),
 + GFP_KERNEL);
 + if (mv_phy-extern_chip.head == NULL)
 + return -ENOMEM;
 + ATOMIC_INIT_NOTIFIER_HEAD(mv_phy-extern_chip.head);

Why do you need to allocate an atomic notifier list head as an entirely
separate data structure?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 05/22] mfd: omap-usb-tll: Clean up clock handling

[Russell King - ARM Linux]

On Mon, Jan 21, 2013 at 01:04:46PM +0200, Roger Quadros wrote:
 Every channel has a functional clock that is similarly named.
 It makes sense to use a for loop to manage these clocks as OMAPs
 can come with up to 3 channels.
 
 Dynamically allocate and get channel clocks depending on the
 number of clocks avaiable on the platform.
 
 Signed-off-by: Roger Quadros rog...@ti.com
 Reviewed-by: Felipe Balbi ba...@ti.com

Much better from the clk API usage, thanks.

Acked-by: Russell King rmk+ker...@arm.linux.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 05/22] mfd: omap-usb-tll: Clean up clock handling

[Russell King - ARM Linux]

On Fri, Jan 18, 2013 at 02:17:08PM +0200, Roger Quadros wrote:
 + tll-ch_clk = devm_kzalloc(dev, sizeof(struct clk * [tll-nch]),
 + GFP_KERNEL);
 + if (!tll-ch_clk) {
 + ret = -ENOMEM;
 + dev_err(dev, Couldn't allocate memory for channel clocks\n);
 + goto err_clk_alloc;
 + }
 +
 + for (i = 0; i  tll-nch; i++) {
 + char clkname[] = usb_tll_hs_usb_chx_clk;
 + struct clk *fck;
 +
 + snprintf(clkname, sizeof(clkname),
 + usb_tll_hs_usb_ch%d_clk, i);
 + fck = clk_get(dev, clkname);
 +
 + if (IS_ERR(fck))
 + dev_dbg(dev, can't get clock : %s\n, clkname);
 + else
 + tll-ch_clk[i] = fck;

Hmm.  I'd actually suggest that this becomes:

tll-ch_clk[i] = fck;
if (IS_ERR(fck)
dev_dbg(dev, can't get clock: %s\n, clkname);

so that tll-ch_clk is always written to.

  static int usbtll_omap_remove(struct platform_device *pdev)
  {
   struct usbtll_omap *tll = platform_get_drvdata(pdev);
 + int i;
 +
 + for (i = 0; i  tll-nch; i++)
 + clk_put(tll-ch_clk[i]);

And change this to:

for (i = 0; i  tll-nch; i++)
if (!IS_ERR(tll-ch_clk[i]))
clk_put(tll-ch_clk[i]);

so that you're not passing a NULL pointer in.

 + for (i = 0; i  tll-nch; i++) {
 + if (is_ehci_tll_mode(pdata-port_mode[i])) {
 + int r;
  
 - if (is_ehci_tll_mode(pdata-port_mode[1]))
 - clk_enable(tll-usbtll_p2_fck);
 + if (!tll-ch_clk[i])

if (IS_ERR(tll-ch_clk[i]))

 + continue;
 +
 + r = clk_enable(tll-ch_clk[i]);
 + if (r) {
 + dev_err(dev,
 +  Error enabling ch %d clock: %d\n, i, r);
 + }
 + }
 + }

 @@ -387,11 +409,12 @@ static int usbtll_runtime_suspend(struct device *dev)
  
   spin_lock_irqsave(tll-lock, flags);
  
 - if (is_ehci_tll_mode(pdata-port_mode[0]))
 - clk_disable(tll-usbtll_p1_fck);
 -
 - if (is_ehci_tll_mode(pdata-port_mode[1]))
 - clk_disable(tll-usbtll_p2_fck);
 + for (i = 0; i  tll-nch; i++) {
 + if (is_ehci_tll_mode(pdata-port_mode[i])) {
 + if (tll-ch_clk[i])

if (!IS_ERR(tll-ch_clk[i]))

 + clk_disable(tll-ch_clk[i]);
 + }
 + }
  
   spin_unlock_irqrestore(tll-lock, flags);
  
 -- 
 1.7.4.1
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 06/22] mfd: omap-usb-tll: introduce and use mode_needs_tll()

[Russell King - ARM Linux]

On Fri, Jan 18, 2013 at 02:17:09PM +0200, Roger Quadros wrote:
 +/* only PHY and UNUSED modes don't need TLL */
 +#define omap_usb_mode_needs_tll(x)   ((x != OMAP_USBHS_PORT_MODE_UNUSED) \
 +  (x != OMAP_EHCI_PORT_MODE_PHY))

Growl.

These parens do not make anything safer at all - they just obfuscate.  The
safe version of the above macro would have been:

#define omap_usb_mode_needs_tll(x)  ((x) != OMAP_USBHS_PORT_MODE_UNUSED \
 (x) != OMAP_EHCI_PORT_MODE_PHY)

If you're going to use a macro argument in an expression, it needs parens.
Simple expressions like a != b  c != d do _not_ need parens.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 04/22] mfd: omap-usb-tll: Clean up clock handling

[Russell King - ARM Linux]

On Wed, Jan 16, 2013 at 04:43:35PM +0200, Roger Quadros wrote:
 + spin_lock_irqsave(tll-lock, flags);
 +
 + for (i = 0; i  tll-nch; i++) {
 + char clkname[] = usb_tll_hs_usb_chx_clk;
 + struct clk *fck;
 +
 + snprintf(clkname, sizeof(clkname),
 + usb_tll_hs_usb_ch%d_clk, i);
 + fck = clk_get(dev, clkname);

NAK.  Why are you doing this under a spinlock?

clk_get() takes a mutex.  You must not be in an atomic region (iow, you
must not be holding a spinlock, and you must not have IRQs disabled)
when you call clk_get().
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/14] usb: phy: nop: Handle power supply regulator for the PHY

[Russell King - ARM Linux]

On Mon, Jan 14, 2013 at 11:54:42AM +0200, Roger Quadros wrote:
 On 01/11/2013 07:17 PM, Russell King - ARM Linux wrote:
  On Thu, Jan 10, 2013 at 06:51:24PM +0200, Roger Quadros wrote:
  We use vcc as the supply name for the PHY's power supply.
  The power supply will be enabled during .init() and disabled
  during .shutdown()
 
  Signed-off-by: Roger Quadros rog...@ti.com
  ---
   drivers/usb/otg/nop-usb-xceiv.c |   18 ++
   1 files changed, 18 insertions(+), 0 deletions(-)
 
  diff --git a/drivers/usb/otg/nop-usb-xceiv.c 
  b/drivers/usb/otg/nop-usb-xceiv.c
  index 163f972..1c6db10 100644
  --- a/drivers/usb/otg/nop-usb-xceiv.c
  +++ b/drivers/usb/otg/nop-usb-xceiv.c
  @@ -33,11 +33,13 @@
   #include linux/usb/nop-usb-xceiv.h
   #include linux/slab.h
   #include linux/clk.h
  +#include linux/regulator/consumer.h
   
   struct nop_usb_xceiv {
 struct usb_phy  phy;
 struct device   *dev;
 struct clk  *clk;
  +  struct regulator*vcc;
   };
   
   static struct platform_device *pd;
  @@ -70,6 +72,11 @@ static int nop_init(struct usb_phy *phy)
   {
 struct nop_usb_xceiv *nop = dev_get_drvdata(phy-dev);
   
  +  if (nop-vcc) {
  +  if (regulator_enable(nop-vcc))
  +  dev_err(phy-dev, Failed to enable power\n);
  +  }
  +
 if (nop-clk)
 clk_enable(nop-clk);
   
  @@ -82,6 +89,11 @@ static void nop_shutdown(struct usb_phy *phy)
   
 if (nop-clk)
 clk_disable(nop-clk);
  +
  +  if (nop-vcc) {
  +  if (regulator_disable(nop-vcc))
  +  dev_err(phy-dev, Failed to disable power\n);
  +  }
   }
   
   static int nop_set_peripheral(struct usb_otg *otg, struct usb_gadget 
  *gadget)
  @@ -157,6 +169,12 @@ static int nop_usb_xceiv_probe(struct platform_device 
  *pdev)
 }
 }
   
  +  nop-vcc = devm_regulator_get(pdev-dev, vcc);
  +  if (IS_ERR(nop-vcc)) {
  +  dev_dbg(pdev-dev, Error getting vcc regulator\n);
  +  nop-vcc = NULL;
  +  }
  
  Is it really appropriate for drivers to do this kind of thing with
  pointer-returning functions (I mean, setting the pointer to NULL on
  error, rather than just using a test for IS_ERR() in the above
  locations).  You are imposing driver-local assumptions on an API.
  
  Practically it probably doesn't make much difference but given the
  amount of mistakes that we have with IS_ERR_OR_NULL()...
  
 Makes sense. I'll convert it to use IS_ERR_OR_NULL() throughout.

IS_ERR_OR_NULL() is going to be deprecated and removed.  Please take a
look at how these APIs work.

If regulator support is not enabled, then devm_regulator_get() returns
NULL, and all the other regulator functions become no-ops.  This is not
an error.  Do you need the driver to error out if CONFIG_REGULATOR is
disabled?

If regulator support is enabled, then it can return a pointer-error
value (as defined when IS_ERR() is true, and _in that case only_ can it
be decoded by PTR_ERR() - PTR_ERR() is _only_ valid when IS_ERR()
returns true - it is _not_ valid for all the cases that IS_ERR_OR_NULL()
returns true.)  Otherwise, it returns a cookie as far as the driver is
concerned for the regulator suitable for passing into the other regulator
APIs.

By having the driver do something different, and make use of NULL as its
own special sentinel, it's placing additional interpretations on the API,
which can lead to bugs.  Don't do this.

If you can start making use of 'nop' vcc/clk members before they've been
got then initialize them to ERR_PTR(-EINVAL) first.

Also consider that when using the devm interfaces, you can do:


nop-clk = devm_clk_get(pdev-dev, ...);
nop-vcc = devm_regulator_get(pdev-dev, vcc);

if (IS_ERR(nop-clk))
dev_dbg(pdev-dev, unable to get clock: %d\n,
PTR_ERR(nop-clk));

if (IS_ERR(nop-vcc))
dev_dbg(pdev-dev, unable to get vcc regulator: %d\n,
PTR_ERR(nop-vcc));
...
if (!IS_ERR(nop-clk))
clk_enable(nop-clk);

You may also consider that if you're going to print a warning for
regulator_enable(), that you should print the error code as well, so that
you know the reason why the failure happened.

Also consider... is dev_err() appropriate for an error, for which you
print a message and continue as if nothing went wrong.  To me that sounds
more like a warning than an error, so maybe dev_warn() would be more
appropriate?
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/14] usb: phy: nop: Handle power supply regulator for the PHY

[Russell King - ARM Linux]

On Mon, Jan 14, 2013 at 01:51:07PM +0200, Roger Quadros wrote:
 On 01/14/2013 01:25 PM, Russell King - ARM Linux wrote:
  Also consider... is dev_err() appropriate for an error, for which you
  print a message and continue as if nothing went wrong.  To me that sounds
  more like a warning than an error, so maybe dev_warn() would be more
  appropriate?
  
 I used dev_dbg(), because we don't treat not getting the power supply
 regulator as that serious.

This comment is about what you do when regulator_enable() and the like
returns non-zero.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] usb: fsl_mxc_udc: replace MX35_IO_ADDRESS to ioremap

[Russell King - ARM Linux]

On Mon, Jan 14, 2013 at 06:12:40PM +0800, Peter Chen wrote:
 @@ -83,15 +84,16 @@ void fsl_udc_clk_finalize(enum fsl_udc_type devtype,
   struct fsl_usb2_platform_data *pdata = pdev-dev.platform_data;
   if (devtype == IMX35_UDC) {
   unsigned int v;
 + void __iomem *phy_regs = ioremap((unsigned long)pdata-regs +
 + MX35_USBPHYCTRL_OFFSET, 512);

Consider that ioremap() can fail.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/14] usb: ehci-omap: Instantiate PHY devices if required

[Russell King - ARM Linux]

On Fri, Jan 11, 2013 at 06:03:21PM +0200, Roger Quadros wrote:
 diff --git a/include/linux/platform_data/usb-omap.h
 b/include/linux/platform_data/usb-omap.h
 index d63eb7d..927b8a1 100644
 --- a/include/linux/platform_data/usb-omap.h
 +++ b/include/linux/platform_data/usb-omap.h
 @@ -38,6 +38,12 @@ enum usbhs_omap_port_mode {
   OMAP_OHCI_PORT_MODE_TLL_2PIN_DPDM
  };
 
 +struct usbhs_phy_config {
 + char *name; /* binds to device driver */

You may wish to consider making this const.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] usb: phy: samsung: Add support to set pmu isolation

[Russell King - ARM Linux]

On Wed, Dec 26, 2012 at 05:58:32PM +0530, Vivek Gautam wrote:
 + if (!ret)
 + sphy-phyctrl_pmureg = ioremap(reg[0], reg[1]);
 +
 + of_node_put(usbphy_pmu);
 +
 + if (IS_ERR_OR_NULL(sphy-phyctrl_pmureg)) {

No.  Learn what the error return values are from functions.  Using the
wrong ones is buggy.  ioremap() only ever returns NULL on error.  You
must check against NULL, and not use the IS_ERR stuff.

 +/*
 + * Set isolation here for phy.
 + * SOCs control this by controlling corresponding PMU registers
 + */
 +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on)
 +{
 + u32 reg;
 + int en_mask;
 +
 + if (!sphy-phyctrl_pmureg) {
 + dev_warn(sphy-dev, Can't set pmu isolation\n);
 + return;
 + }
 +
 + reg = readl(sphy-phyctrl_pmureg);
 +
 + en_mask = sphy-drv_data-devphy_en_mask;
 +
 + if (on)
 + writel(reg  ~en_mask, sphy-phyctrl_pmureg);
 + else
 + writel(reg | en_mask, sphy-phyctrl_pmureg);

What guarantees that this read-modify-write sequence of this register safe?
And, btw, this can't be optimised very well because of the barrier inside
writel().  This would be better:

if (on)
reg = ~en_mask;
else
reg |= en_mask;

writel(reg, sphy-phyctrl_pmureg);

 +static inline struct samsung_usbphy_drvdata
 +*samsung_usbphy_get_driver_data(struct platform_device *pdev)
  {
   if (pdev-dev.of_node) {
   const struct of_device_id *match;
   match = of_match_node(samsung_usbphy_dt_match,
   pdev-dev.of_node);
 - return (int) match-data;
 + return (struct samsung_usbphy_drvdata *) match-data;

match-data is a const void pointer.  Is there a reason you need this
cast here?  What if you made the returned pointer from this function
also const and fixed up all its users (no user should modify this
data.)

  #ifdef CONFIG_OF
  static const struct of_device_id samsung_usbphy_dt_match[] = {
   {
   .compatible = samsung,s3c64xx-usbphy,
 - .data = (void *)TYPE_S3C64XX,
 + .data = (void *)usbphy_s3c64xx,

Why do you need this cast?

   }, {
   .compatible = samsung,exynos4210-usbphy,
 - .data = (void *)TYPE_EXYNOS4210,
 + .data = (void *)usbphy_exynos4,

Ditto.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html