Re: [PATCH v4] Move DWC2 driver out of staging
On Fri, Jan 31, 2014 at 11:48:37PM -0700, Stephen Warren wrote:
> On 01/31/2014 11:12 AM, Andre Heider wrote:
> > On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
> >> The DWC2 driver should now be in good enough shape to move out of
> >> staging. I have stress tested it overnight on RPI running mass
> >> storage and Ethernet transfers in parallel, and for several days
> >> on our proprietary PCI-based platform.
> ...
> > this looks just fine, but for whatever reason it breaks sdhci on my rpi.
> > With today's Linus' master the dwc2 controller seems to initialize fine,
> > but I get this upon boot:
> >
> > [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
> > [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
> >
> > That is:
> >
> > struct sdhci_host *sdhci_pltfm_init(struct platform_device
> > *pdev,
> > const struct
> > sdhci_pltfm_data *pdata,
> > size_t priv_size)
> > {
> > ...
> >
> > iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!iomem) {
> > ret = -ENOMEM;
> > goto err;
> > }
>
> This is due to the following code:
>
> static void _dwc2_hcd_endpoint_reset(struct usb_hcd *hcd,
>struct usb_host_endpoint *ep)
> {
> struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> ...
> struct usb_device *udev;
> ...
> udev = to_usb_device(hsotg->dev);
> ...
> usb_settoggle(udev, epnum, is_out, 0);
> if (is_control)
> usb_settoggle(udev, epnum, !is_out, 0);
>
> The problem is that hsotg->dev is assigned as follows:
>
> static int dwc2_driver_probe(struct platform_device *dev)
> ...
> hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
> ...
> hsotg->dev = &dev->dev;
>
> As such, it's not legal to call to_usb_device() on it.
>
> What ends up happening, simply due to memory allocation order, is that
> the memory writes inside usb_settoggle() end up setting the SDHCI struct
> platform_device's num_resources to 0, so that it's call to
> platform_get_resource() fails.
>
> With the DWC2 move patch reverted, some other random piece of memory is
> being corrupted, which just happens not to cause any visible problem.
> Likely it's some other struct platform_device that's already had its
> resources read by the time DWC2 probes and corrupts them.
>
> (Yes, this was hard to find!)
Nice work, but how did you pinpoint this? Am I missing some option/tool
or did I just not stare for long enough?
> I honestly can't see how to solve this myself, since the whole DWC2
> driver doesn't seem to have a struct usb_device * hanging around that we
> can stash somewhere for it to look up later. Perhaps someone more
> familiar with the USB stack can help with that.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union
On 01/25/2014 07:55 PM, Chen Gang wrote: > On 01/21/2014 06:36 PM, James Hogan wrote: >> Hi Dan, >> >> On 20/01/14 21:13, Dan Carpenter wrote: >>> I made a quick and dirty sparse patch to check for this. I don't think >>> I will bother trying to send it to sparse upstream, but you can if you >>> want to. >>> >>> It found 289 unions which might need a __packed added. The lustre >>> unions were not in my allmodconfig so they're not listed. >> >> Thanks a lot for this, it seems to be useful. I'm adapting it to reduce >> false negatives (e.g. omitting the check if the struct/union is already >> packed), and I imagine it could be made to only warn about padded >> unpacked structs/unions which are used as nested members of packed >> structs/unions. It wouldn't catch everything but would probably catch a >> lot of cases that are most likely to be genuine since they would have >> been packed at the outer level for a reason. >> >>> Perhaps there could be a command line option or a pragma so that unions >>> will work in the kernel. We don't care about linking to outside >>> libraries. >> >> We still interact with userland via structs and unions, so it would >> probably have to exclude anything in uapi/. >> > It seems, our kernel still stick to treate 'pack' region have effect with both 'align' and 'sizeof'. So for compiler, could we add one additional cflag parameter to tell compiler to switch it (compatible with ABI, or satisfy upstream kernel). And for kernel, it will be OK enough to only append this parameter to KBUILD_CFLAGS in "arch/metag/Makefile". Thanks. > Thank all of you firstly. > > But excuse me, I am still not quit clear that: "what need we do enough > to solve this feature issue?" > > So I guess our current result is: > > - It is not a good idea to only let kernel to fit with compiler. > > - It is not a good idea to only let compiler to fit with kernel. > > - Need let compiler and kernel to fit with each other: > >- compiler will print related warning, but not break compiling. > so metag compiler need be improvement (check and warn for it). > >- if check alignment explicitly in kernel source code, it need be > fixed within kernel: "apply related patches (pack each struct or > union), but the related patch comments need be improved". > > Is what I guess correct? > > Thanks. > -- Chen Gang Open, share and attitude like air, water and life which God blessed ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Move DWC2 driver out of staging
On Fri, 31 Jan 2014, Stephen Warren wrote:
> This is due to the following code:
>
> static void _dwc2_hcd_endpoint_reset(struct usb_hcd *hcd,
>struct usb_host_endpoint *ep)
> {
> struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> ...
> struct usb_device *udev;
> ...
> udev = to_usb_device(hsotg->dev);
> ...
> usb_settoggle(udev, epnum, is_out, 0);
> if (is_control)
> usb_settoggle(udev, epnum, !is_out, 0);
>
> The problem is that hsotg->dev is assigned as follows:
>
> static int dwc2_driver_probe(struct platform_device *dev)
> ...
> hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
> ...
> hsotg->dev = &dev->dev;
>
> As such, it's not legal to call to_usb_device() on it.
This clearly is a bug. I suspect the driver has other bugs too, such
as not holding the private lock over a large enough region.
> What ends up happening, simply due to memory allocation order, is that
> the memory writes inside usb_settoggle() end up setting the SDHCI struct
> platform_device's num_resources to 0, so that it's call to
> platform_get_resource() fails.
>
> With the DWC2 move patch reverted, some other random piece of memory is
> being corrupted, which just happens not to cause any visible problem.
> Likely it's some other struct platform_device that's already had its
> resources read by the time DWC2 probes and corrupts them.
>
> (Yes, this was hard to find!)
>
> I honestly can't see how to solve this myself, since the whole DWC2
> driver doesn't seem to have a struct usb_device * hanging around that we
> can stash somewhere for it to look up later. Perhaps someone more
> familiar with the USB stack can help with that.
The correct solution is to set
qh = ep->hcpriv;
udev = qh->udev;
However, the driver doesn't store udev in qh (dwc2_qh_init() should do
this, but it doesn't). In fact, the field doesn't even exist.
Alan Stern
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [GIT PULL] Staging wireless driver for 3.14-rc1
On Fri, Jan 31, 2014 at 6:32 AM, Greg KH wrote:
>
> Here's a single staging driver for a wireless chipset that has shown up
> in the SteamBox hardware. It is merged separately from the "main"
> staging pull request to sync up with the wireless api changes that came
> in from the networking tree.
It causes an interesting warning for me:
drivers/staging/rtl8821ae/rtl8821ae/dm.c: In function
‘rtl8821ae_dm_clear_txpower_tracking_state’:
drivers/staging/rtl8821ae/rtl8821ae/dm.c:487:31: warning: iteration 2u
invokes undefined behavior [-Waggressive-loop-optimizations]
rtldm->bb_swing_idx_ofdm[p] = rtldm->default_ofdm_index;
^
drivers/staging/rtl8821ae/rtl8821ae/dm.c:485:2: note: containing loop
for (p = RF90_PATH_A; p < MAX_RF_PATH; ++p) {
^
and gcc is entirely correct: that loop iterates from 0 to 3, and does this:
rtldm->bb_swing_idx_ofdm[p] = rtldm->default_ofdm_index;
but the bb_swing_idx_ofdm[] array only has two members. So the last
two iterations will overwrite bb_swing_idx_ofdm_current and the first
entry in bb_swing_idx_ofdm_base[].
Now, the bug does seem to be benign: bb_swing_idx_ofdm_current isn't
actually ever *used* as far as I can tell, and the first entry of
bb_swing_idx_ofdm_base[] will have been written with that same
"rtldm->default_ofdm_index" value.
But gcc is absolutely correct, and that driver needs fixing.
I've pulled it and will let it be because it doesn't seem to be an
issue in practice, but please fix it. The obvious fix would seem to
change the size of "2" to be "MAX_RF_PATH", but I'll abstain from
doing those kinds of changes in the merge when it doesn't seem to
affect the build or functionality).
Linus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] drivers:staging:tl8821ae Fixed the size of array to macro
Changed array size from "bb_swing_idx_ofdm[2]" to
"bb_swing_idx_ofdm[MAX_RF_PATH]"
as discussed on thread - https://lkml.org/lkml/2014/2/1/75
Signed-off-by: Surendra Patil
---
drivers/staging/rtl8821ae/wifi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8821ae/wifi.h b/drivers/staging/rtl8821ae/wifi.h
index cfe88a1..76bef93 100644
--- a/drivers/staging/rtl8821ae/wifi.h
+++ b/drivers/staging/rtl8821ae/wifi.h
@@ -1414,7 +1414,7 @@ struct rtl_dm {
/*88e tx power tracking*/
- u8 bb_swing_idx_ofdm[2];
+ u8 bb_swing_idx_ofdm[MAX_RF_PATH];
u8 bb_swing_idx_ofdm_current;
u8 bb_swing_idx_ofdm_base[MAX_RF_PATH];
bool bb_swing_flag_Ofdm;
--
1.8.3.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/2] Drivers: net: hyperv: Cleanup the recive path
From: "K. Y. Srinivasan" Date: Fri, 31 Jan 2014 08:24:38 -0800 > Some minor cleanup of the receive path. Get rid of unnecessary > indirection as well as unnecessary re-establishment of state. It is not appropriate to submit cleanups at this time. Please wait until the net-next tree opens back up, and submit your changes against it at that time. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/1] Drivers: hv: vmbus: Cleanup the packet send path
The current channel code is using scatterlist abstraction to pass data to the
ringbuffer API on the send path. This causes unnecessary translations
between virtual and physical addresses. Fix this.
Signed-off-by: K. Y. Srinivasan
---
drivers/hv/channel.c | 42 +++---
drivers/hv/hyperv_vmbus.h |4 ++--
drivers/hv/ring_buffer.c | 17 +++--
include/linux/hyperv.h|2 +-
4 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 69ea36f..602ca86 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "hyperv_vmbus.h"
@@ -554,14 +555,14 @@ EXPORT_SYMBOL_GPL(vmbus_close);
*
* Mainly used by Hyper-V drivers.
*/
-int vmbus_sendpacket(struct vmbus_channel *channel, const void *buffer,
+int vmbus_sendpacket(struct vmbus_channel *channel, void *buffer,
u32 bufferlen, u64 requestid,
enum vmbus_packet_type type, u32 flags)
{
struct vmpacket_descriptor desc;
u32 packetlen = sizeof(struct vmpacket_descriptor) + bufferlen;
u32 packetlen_aligned = ALIGN(packetlen, sizeof(u64));
- struct scatterlist bufferlist[3];
+ struct kvec bufferlist[3];
u64 aligned_data = 0;
int ret;
bool signal = false;
@@ -575,11 +576,12 @@ int vmbus_sendpacket(struct vmbus_channel *channel, const
void *buffer,
desc.len8 = (u16)(packetlen_aligned >> 3);
desc.trans_id = requestid;
- sg_init_table(bufferlist, 3);
- sg_set_buf(&bufferlist[0], &desc, sizeof(struct vmpacket_descriptor));
- sg_set_buf(&bufferlist[1], buffer, bufferlen);
- sg_set_buf(&bufferlist[2], &aligned_data,
- packetlen_aligned - packetlen);
+ bufferlist[0].iov_base = &desc;
+ bufferlist[0].iov_len = sizeof(struct vmpacket_descriptor);
+ bufferlist[1].iov_base = buffer;
+ bufferlist[1].iov_len = bufferlen;
+ bufferlist[2].iov_base = &aligned_data;
+ bufferlist[2].iov_len = (packetlen_aligned - packetlen);
ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal);
@@ -605,7 +607,7 @@ int vmbus_sendpacket_pagebuffer(struct vmbus_channel
*channel,
u32 descsize;
u32 packetlen;
u32 packetlen_aligned;
- struct scatterlist bufferlist[3];
+ struct kvec bufferlist[3];
u64 aligned_data = 0;
bool signal = false;
@@ -637,11 +639,12 @@ int vmbus_sendpacket_pagebuffer(struct vmbus_channel
*channel,
desc.range[i].pfn= pagebuffers[i].pfn;
}
- sg_init_table(bufferlist, 3);
- sg_set_buf(&bufferlist[0], &desc, descsize);
- sg_set_buf(&bufferlist[1], buffer, bufferlen);
- sg_set_buf(&bufferlist[2], &aligned_data,
- packetlen_aligned - packetlen);
+ bufferlist[0].iov_base = &desc;
+ bufferlist[0].iov_len = descsize;
+ bufferlist[1].iov_base = buffer;
+ bufferlist[1].iov_len = bufferlen;
+ bufferlist[2].iov_base = &aligned_data;
+ bufferlist[2].iov_len = (packetlen_aligned - packetlen);
ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal);
@@ -665,7 +668,7 @@ int vmbus_sendpacket_multipagebuffer(struct vmbus_channel
*channel,
u32 descsize;
u32 packetlen;
u32 packetlen_aligned;
- struct scatterlist bufferlist[3];
+ struct kvec bufferlist[3];
u64 aligned_data = 0;
bool signal = false;
u32 pfncount = NUM_PAGES_SPANNED(multi_pagebuffer->offset,
@@ -700,11 +703,12 @@ int vmbus_sendpacket_multipagebuffer(struct vmbus_channel
*channel,
memcpy(desc.range.pfn_array, multi_pagebuffer->pfn_array,
pfncount * sizeof(u64));
- sg_init_table(bufferlist, 3);
- sg_set_buf(&bufferlist[0], &desc, descsize);
- sg_set_buf(&bufferlist[1], buffer, bufferlen);
- sg_set_buf(&bufferlist[2], &aligned_data,
- packetlen_aligned - packetlen);
+ bufferlist[0].iov_base = &desc;
+ bufferlist[0].iov_len = descsize;
+ bufferlist[1].iov_base = buffer;
+ bufferlist[1].iov_len = bufferlen;
+ bufferlist[2].iov_base = &aligned_data;
+ bufferlist[2].iov_len = (packetlen_aligned - packetlen);
ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, &signal);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index e055176..1544609 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -559,8 +559,8 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info
*ring_info, void *buffer,
void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info);
int hv_ringbuffer_write(struct hv_ring_buffer_info *ring_info,
- struct scatterlist *sglist,
- u32 sgcount, bool *signal);
+
Re: [PATCH v4] Move DWC2 driver out of staging
On 02/01/2014 03:00 AM, Andre Heider wrote: > On Fri, Jan 31, 2014 at 11:48:37PM -0700, Stephen Warren wrote: >> On 01/31/2014 11:12 AM, Andre Heider wrote: >>> On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote: The DWC2 driver should now be in good enough shape to move out of staging. I have stress tested it overnight on RPI running mass storage and Ethernet transfers in parallel, and for several days on our proprietary PCI-based platform. >> ... >>> this looks just fine, but for whatever reason it breaks sdhci on my rpi. >>> With today's Linus' master the dwc2 controller seems to initialize fine, >>> but I get this upon boot: >>> >>> [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12 >>> [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12 ... >> This is due to the following code: ... >> What ends up happening, simply due to memory allocation order, is that >> the memory writes inside usb_settoggle() end up setting the SDHCI struct >> platform_device's num_resources to 0, so that it's call to >> platform_get_resource() fails. >> >> With the DWC2 move patch reverted, some other random piece of memory is >> being corrupted, which just happens not to cause any visible problem. >> Likely it's some other struct platform_device that's already had its >> resources read by the time DWC2 probes and corrupts them. >> >> (Yes, this was hard to find!) > > Nice work, but how did you pinpoint this? Am I missing some option/tool > or did I just not stare for long enough? Well, there was a clear place where an issue was present; the resource lookup in sdhci_pltfm_init() was failing, so I put a bunch of printfs into that function to dump out the data platform_get_resource() used. This clearly pointed at num_resources==0 being the problem. Next, I dumped the same data from the code in drivers/of that sets it up, and it was OK there, so I knew it was getting over-written somewhere. I then basically added hundreds of calls to the same data dumping function throughout kernel functions like really_probe() to track down the location of the problem. Luckily, the behaviour was stable, so I wasn't chasing a race/timing condition. Eventually I narrowed the window to the few lines of code I mentioned in _dwc2_hcd_endpoint_reset(). It would have been much harder if it was e.g. the USB HW DMAing to memory that caused the corruption, so I was lucky:-) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [GIT PULL] Staging wireless driver for 3.14-rc1
On Sat, Feb 01, 2014 at 10:43:11AM -0800, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 6:32 AM, Greg KH wrote:
> >
> > Here's a single staging driver for a wireless chipset that has shown up
> > in the SteamBox hardware. It is merged separately from the "main"
> > staging pull request to sync up with the wireless api changes that came
> > in from the networking tree.
>
> It causes an interesting warning for me:
>
> drivers/staging/rtl8821ae/rtl8821ae/dm.c: In function
> ‘rtl8821ae_dm_clear_txpower_tracking_state’:
> drivers/staging/rtl8821ae/rtl8821ae/dm.c:487:31: warning: iteration 2u
> invokes undefined behavior [-Waggressive-loop-optimizations]
>rtldm->bb_swing_idx_ofdm[p] = rtldm->default_ofdm_index;
>^
> drivers/staging/rtl8821ae/rtl8821ae/dm.c:485:2: note: containing loop
> for (p = RF90_PATH_A; p < MAX_RF_PATH; ++p) {
> ^
>
> and gcc is entirely correct: that loop iterates from 0 to 3, and does this:
>
> rtldm->bb_swing_idx_ofdm[p] = rtldm->default_ofdm_index;
>
> but the bb_swing_idx_ofdm[] array only has two members. So the last
> two iterations will overwrite bb_swing_idx_ofdm_current and the first
> entry in bb_swing_idx_ofdm_base[].
>
> Now, the bug does seem to be benign: bb_swing_idx_ofdm_current isn't
> actually ever *used* as far as I can tell, and the first entry of
> bb_swing_idx_ofdm_base[] will have been written with that same
> "rtldm->default_ofdm_index" value.
>
> But gcc is absolutely correct, and that driver needs fixing.
>
> I've pulled it and will let it be because it doesn't seem to be an
> issue in practice, but please fix it. The obvious fix would seem to
> change the size of "2" to be "MAX_RF_PATH", but I'll abstain from
> doing those kinds of changes in the merge when it doesn't seem to
> affect the build or functionality).
Ok, thanks, someone's already sent me that patch to fix this up, I'll
queue it up for the post-rc1 set of staging tree fixes.
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
