Re: rtl8821ae.

2014-02-05 Thread Andrea Merello 
>
> Somehow I missed this patch. Do you know if John Linville picked it up?
>

AFAIK no..

> I'm not sure if the ANAPARAM3 register could handle a 16-bit write on an
> RTL8187B. To be cautious, I wrote and have tested the attached patch using a
> union. The patch includes a fix for an undefined symbol
> (RTL818X_TX_CONF_HW_SEQNUM). With this patch my RTL8187SE and RTL8187B
> devices both work.

Excellent, thank you !


> Larry
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] et131x: fix allocation failures

2014-02-05 Thread Zhao, Gang 
add Cc to devel mailing list

On Wed, 2014-02-05 at 19:56:19 +0800, Alan wrote:
> We should check the ring allocations don't fail.
> If we get a fail we need to clean up properly. The allocator assumes the
> deallocator will be used on failure, but it isn't. Fix this and add a
> missing check against fbr allocation failure.
>
> Signed-off-by: Alan Cox 
> ---
>  drivers/staging/et131x/et131x.c |9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index e516bb6..26da8c8 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct 
> et131x_adapter *adapter)
>  
>   /* Alloc memory for the lookup table */
>   rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> + if (rx_ring->fbr[0] == NULL)
> + return -ENOMEM;
>   rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> + if (rx_ring->fbr[1])

Hi, I think it should be:

if (!rx_ring->fbr[1]) {
kfree(rx_ring->fbr[0]);
return -ENOMEM;
}

The other changes seem good.

> + return -ENOMEM;
>  
>   /* The first thing we will do is configure the sizes of the buffer
>* rings. These will change based on jumbo packet support.  Larger
> @@ -2289,7 +2293,7 @@ static void et131x_rx_dma_memory_free(struct 
> et131x_adapter *adapter)
>   for (id = 0; id < NUM_FBRS; id++) {
>   fbr = rx_ring->fbr[id];
>  
> - if (!fbr->ring_virtaddr)
> + if (!fbr || !fbr->ring_virtaddr)
>   continue;
>  
>   /* First the packet memory */
> @@ -3591,6 +3595,7 @@ static int et131x_adapter_memory_alloc(struct 
> et131x_adapter *adapter)
>   if (status) {
>   dev_err(&adapter->pdev->dev,
> "et131x_tx_dma_memory_alloc FAILED\n");
> + et131x_tx_dma_memory_free(adapter);
>   return status;
>   }
>   /* Receive buffer memory allocation */
> @@ -3598,7 +3603,7 @@ static int et131x_adapter_memory_alloc(struct 
> et131x_adapter *adapter)
>   if (status) {
>   dev_err(&adapter->pdev->dev,
> "et131x_rx_dma_memory_alloc FAILED\n");
> - et131x_tx_dma_memory_free(adapter);
> + et131x_adapter_memory_free(adapter);
>   return status;
>   }
>  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: rtl8821ae.

2014-02-05 Thread Andrea Merello 
> Looks good.  It would be best to submit the bugfixes first like where
> you add error checking for pci_map_single().

Yes, this is exactly what I started to do this morning :)
I'm preparing a patchseries where I extracting fixes or improvements
unrelated to rtl8187se, including pci_map_single fixes

> You add too many blank lines btw, you should never have two
> consecutive blank lines.  Don't add a blank line in the middle of the
> declaration block or before a close curly brace '}' line.

OK

> After that
> maybe the patch to change "if (priv->r8185)" to
> "if (priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185)". Then the patch
> to add rtl8225se support.

Yes, this is also my plan :)

> Some minor things below.

Ok, I have reviewed all your advices.
I agree with them.
I also commended the ones where you asked something about.

> regards,
> dan carpenter

Thank you a lot for your review!

Andrea

>> +#include "rtl8225se.h"
>> +
>
> Don't include this twice.  Only put one blank line, not two consecutive
> blank lines.

OK

> This comment is out of date.  RTL8180_NR_TX_QUEUES is 2.  I haven't
> really understood the changes to rtl8180_queues_map[].  It used to have
> 4 elements and now it only has 2.  Is this change needed to support
> rtl8225se or is it a separate bugfix which should go in a separate
> patch?

It's for rtl8187se.
That card uses more queues to enable WMM

> Also this change which I have pulled out of context:
>
> + if (ring->entries - skb_queue_len(&ring->queue) < 2) {
> + if (prio != dev->queues)
> + ieee80211_stop_queue(dev, prio);
> + else
> + priv->beacon_queue_stop = 1;
> + }
>
> This is a separate bugfix?

Yes, the old driver uses ieee80211_stop_queue and ieee80211_wake_queue
on the beacon queue, that is indeed handled in the driver and not by
ieee80211
This one of the things that I will include in patches for
rtl8180/rtl8185 fixes before merging real rtl8187se code

>> +static void rtl8180_write_phy_(struct ieee80211_hw *dev, u8 addr, u32 data)
>
> This is a temporary name to not conflight with rtl8180_write_phy()?

No, this is the aux function that does the real work, but should not
be called directly in the driver code, as rtl8180_write_phy() should
be called.
Maybe I have to move the underscore at the beginning of the name.

>
> These don't go over the 80 character limit so we don't need the line
> breaks.  The casts are not needed either.

OK

>> + if ((addr == 0xa) && (
>> + priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185))
>> + rb_mask = 0x7f;
>
> Write it like this:
>
> if (addr == 0xa && priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185)
> rb_mask = 0x7f;
>
> If you really want to add parenthesis then do this:
>
> if ((addr == 0xa) &&
> (priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185))
> rb_mask = 0x7f;

OK

>> - buf = (data << 8) | addr;
>> + if (tmp & 0x80)
>> + printk(KERN_WARNING
>> +"rtl818x failed to write BB (busy) adr:%x data:%x\n",
>> +addr, data);
>
> Checkpatch.pl will complain that you should use netdev_warn() or
> something.

OK


>
> Don't put a blank line here in the middle of the declaration block.
>

OK


>> + frame_duration =  priv->ack_time + le16_to_cpu(
>> + ieee80211_generic_frame_duration(dev, priv->vif,
>> + IEEE80211_BAND_2GHZ, skb->len,
>> + ieee80211_get_tx_rate(dev, info)));
>
> Use a temporary variable:
>
> __le16 duration;

I agree. Will do.

>> + /* must be written last */
>>   entry->flags = cpu_to_le32(tx_flags);
>
> Why must this be written last?  The CPU can re-order it anyway unless
> there is some locking.

Because that marks the descriptor as good for the NIC to be processed.
I suppose a wmb() is needed before this line to ensure it is the last written
and another wmb() is needed to ensure that the descriptor is fully
written before polling the NIC for performing DMA (I think that,
depending by the situation, the HW may wait for this register write on
not).


>> + /* Enable DA10 TX power saving */
>> + rtl818x_iowrite8(priv, &priv->map->PHY_PR,
>> +  rtl818x_ioread8(priv, &priv->map->PHY_PR) | 0x04);
>
> Use a temporary variable:
>
> reg = rtl818x_ioread8(priv, &priv->map->PHY_PR);
> rtl818x_iowrite8(priv, &priv->map->PHY_PR, reg | 0x40);
>

OK

>> +static void rtl8187se_set_antenna_config(struct ieee80211_hw *dev, u8 
>> def_ant,
>> +  bool use_diver)
>
> Could you rename "use_diver" to "diversity"?  I misread it every time.

It makes sense.


>> +#if 0
>> +static void rtl8187se_power_tracking(struct ieee80211_hw *dev)
>> +{
>> + struct rtl8180_priv *priv = dev->priv;
>> + u8 tmp = rtl818x_ioread8(priv

[PATCH RESEND 1/3] staging: et131x: stop read when hit max delay in et131x_phy_mii_read

2014-02-05 Thread Zhao, Gang 
stop read and return error when hit max delay time.

Signed-off-by: Zhao, Gang 
Acked-by: Mark Einon 
---

Acked at post: http://article.gmane.org/gmane.linux.kernel/1599347

 drivers/staging/et131x/et131x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index e516bb6..cf97049 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1388,6 +1388,7 @@ static int et131x_phy_mii_read(struct et131x_adapter 
*adapter, u8 addr,
mii_indicator);
 
status = -EIO;
+   goto out;
}
 
/* If we hit here we were able to read the register and we need to
@@ -1395,6 +1396,7 @@ static int et131x_phy_mii_read(struct et131x_adapter 
*adapter, u8 addr,
 */
*value = readl(&mac->mii_mgmt_stat) & ET_MAC_MIIMGMT_STAT_PHYCRTL_MASK;
 
+out:
/* Stop the read operation */
writel(0, &mac->mii_mgmt_cmd);
 
-- 
1.8.5.3

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RESEND 2/3] staging: et131x: remove spinlock adapter->lock

2014-02-05 Thread Zhao, Gang 
adapter->lock is only used in et131x_multicast(), which is eventually
called by network stack function __dev_set_rx_mode(). __dev_set_rx_mode()
is always called by (net_device *)dev->addr_list_lock hold, to protect from
concurrent access. So adapter->lock is redundant.

Signed-off-by: Zhao, Gang 
Acked-by: Mark Einon 
---

Acked at post: http://article.gmane.org/gmane.linux.kernel/1599348

This patch is slightly modified:
also remove unused variable 'unsigned long flags', or this patch will made
sparse warning:
drivers/staging/et131x/et131x.c: In function ‘et131x_multicast’:
drivers/staging/et131x/et131x.c:4289:16: warning: unused variable ‘flags’ 
[-Wunused-variable]
  unsigned long flags;
^

 drivers/staging/et131x/et131x.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index cf97049..6413500 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -485,8 +485,6 @@ struct et131x_adapter {
u8 eeprom_data[2];
 
/* Spinlocks */
-   spinlock_t lock;
-
spinlock_t tcb_send_qlock;
spinlock_t tcb_ready_qlock;
spinlock_t send_hw_lock;
@@ -3762,7 +3760,6 @@ static struct et131x_adapter *et131x_adapter_init(struct 
net_device *netdev,
adapter->netdev = netdev;
 
/* Initialize spinlocks here */
-   spin_lock_init(&adapter->lock);
spin_lock_init(&adapter->tcb_send_qlock);
spin_lock_init(&adapter->tcb_ready_qlock);
spin_lock_init(&adapter->send_hw_lock);
@@ -4294,12 +4291,9 @@ static void et131x_multicast(struct net_device *netdev)
 {
struct et131x_adapter *adapter = netdev_priv(netdev);
int packet_filter;
-   unsigned long flags;
struct netdev_hw_addr *ha;
int i;
 
-   spin_lock_irqsave(&adapter->lock, flags);
-
/* Before we modify the platform-independent filter flags, store them
 * locally. This allows us to determine if anything's changed and if
 * we even need to bother the hardware
@@ -4351,8 +4345,6 @@ static void et131x_multicast(struct net_device *netdev)
 */
if (packet_filter != adapter->packet_filter)
et131x_set_packet_filter(adapter);
-
-   spin_unlock_irqrestore(&adapter->lock, flags);
 }
 
 /* et131x_tx - The handler to tx a packet on the device */
-- 
1.8.5.3

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RESEND 3/3] staging: et131x: fix make W=1 build warning

2014-02-05 Thread Zhao, Gang 
fix make W=1 warning:
drivers/staging/et131x/et131x.c: In function 
‘et1310_setup_device_for_multicast’:
drivers/staging/et131x/et131x.c:1052:6: warning: variable ‘pm_csr’ set but not 
used [-Wunused-but-set-variable]
  u32 pm_csr;
  ^
drivers/staging/et131x/et131x.c: In function ‘et1310_setup_device_for_unicast’:
drivers/staging/et131x/et131x.c:1101:6: warning: variable ‘pm_csr’ set but not 
used [-Wunused-but-set-variable]
  u32 pm_csr;
  ^
drivers/staging/et131x/et131x.c: In function ‘et131x_isr_handler’:
drivers/staging/et131x/et131x.c:4006:8: warning: variable ‘pm_csr’ set but not 
used [-Wunused-but-set-variable]
u32 pm_csr;
^

Signed-off-by: Zhao, Gang 
---
 drivers/staging/et131x/et131x.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 6413500..0c44014 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1049,7 +1049,6 @@ static void et1310_setup_device_for_multicast(struct 
et131x_adapter *adapter)
u32 hash2 = 0;
u32 hash3 = 0;
u32 hash4 = 0;
-   u32 pm_csr;
 
/* If ET131X_PACKET_TYPE_MULTICAST is specified, then we provision
 * the multi-cast LIST.  If it is NOT specified, (and "ALL" is not
@@ -1083,7 +1082,6 @@ static void et1310_setup_device_for_multicast(struct 
et131x_adapter *adapter)
}
 
/* Write out the new hash to the device */
-   pm_csr = readl(&adapter->regs->global.pm_csr);
if (!et1310_in_phy_coma(adapter)) {
writel(hash1, &rxmac->multi_hash1);
writel(hash2, &rxmac->multi_hash2);
@@ -1098,7 +1096,6 @@ static void et1310_setup_device_for_unicast(struct 
et131x_adapter *adapter)
u32 uni_pf1;
u32 uni_pf2;
u32 uni_pf3;
-   u32 pm_csr;
 
/* Set up unicast packet filter reg 3 to be the first two octets of
 * the MAC address for both address
@@ -1124,7 +1121,6 @@ static void et1310_setup_device_for_unicast(struct 
et131x_adapter *adapter)
  (adapter->addr[4] << ET_RX_UNI_PF_ADDR1_5_SHIFT) |
   adapter->addr[5];
 
-   pm_csr = readl(&adapter->regs->global.pm_csr);
if (!et1310_in_phy_coma(adapter)) {
writel(uni_pf1, &rxmac->uni_pf_addr1);
writel(uni_pf2, &rxmac->uni_pf_addr2);
@@ -4003,12 +3999,9 @@ static void et131x_isr_handler(struct work_struct *work)
 */
if (adapter->flowcontrol == FLOW_TXONLY ||
adapter->flowcontrol == FLOW_BOTH) {
-   u32 pm_csr;
-
/* Tell the device to send a pause packet via the back
 * pressure register (bp req and bp xon/xoff)
 */
-   pm_csr = readl(&iomem->global.pm_csr);
if (!et1310_in_phy_coma(adapter))
writel(3, &iomem->txmac.bp_ctrl);
}
-- 
1.8.5.3

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: vme_tsi148 question

2014-02-05 Thread Greg KH 
On Tue, Feb 04, 2014 at 06:34:13PM +, Martyn Welch wrote:
> On 04/02/14 16:34, Michael Kenney wrote:
> >Hi Martyn,
> >
> >On Tue, Feb 4, 2014 at 7:28 AM, Martyn Welch  >> wrote:
> >
> >On 28/12/13 00:34, Michael Kenney wrote:
> >
> >Hi Martyn,
> >
> >On Fri, Dec 27, 2013 at 4:23 PM, Martyn Welch
> >mailto:mar...@welchs.me.uk>> wrote:
> >
> >On 27/12/13 20:15, Michael Kenney wrote:
> >
> >
> >We are using the vme_tsi148 bridge driver along with the
> >vme_user
> >driver to access the VME boards. The A/D board requires
> >D32 bus cycles
> >and the VME master window is configured accordingly,
> >however, when
> >monitoring the bus cycles with a logic analyzer, we
> >noticed that the
> >CPU is transferring one byte at a time (i.e. four D8
> >transfers rather
> >than one D32).
> >
> >Is this the expected behavior of the tsi148 driver?
> >
> >
> >Hi Mike,
> >
> >This is certainly not the expected behaviour - if the window
> >is configured
> >for D32 then it should do 32 bit transfers where possible.
> >
> >I've heard of this happening recently, but haven't yet been
> >able to
> >replicate it. Which VME board are you running Linux on and
> >which flavour of
> >Linux?
> >
> >
> >I'm running Debian 7.2 with kernel 3.2 on a Fastwel CPC600
> >(Pentium M
> >based CPU board).
> >
> >
> >I haven't forgotten about this, still not sure exactly what is
> >happening.
> >
> >Is your install/kernel 32 or 64 bit?
> >
> >Are you doing single 32-bit transfers, or are you seeing this on
> >longer transfers (i.e. copying a buffer full of data)?
> >
> >
> >Thanks for getting back to me.
> >
> >I'm running a 32-bit kernel and I see this behavior on all transfers
> >regardless of buffer size.
> >
> 
> Gah! Thought I could see what may be causing it in 64-bit kernels, but not
> 32-bit (my x86 asm is not particularly hot).
> 
> I think we /may/ be hitting issues with how the memcpy function gets
> implemented on specific architectures. The tsi148 is a PCI/X to VME bridge,
> if it receives a series of 8-bit reads or writes, it translates these to
> 8-bit reads or writes on the VME bus, which is not necessarily what we want.
> 
> According to the data sheet, the card you are using has an Intel 6300ESB
> ICH, which seems to be PCI/X (so we can rule out PCIe to PCI/X bridges or
> something like that doing nasty things).
> 
> I think (if I follow everything correctly) then the memcpy for 32-bit is
> handled by __memcpy in arch/x86/include/asm/string_32.h:
> 
> static __always_inline void *__memcpy(void *to, const void *from, size_t n)
> {
> int d0, d1, d2;
> asm volatile("rep ; movsl\n\t"
>  "movl %4,%%ecx\n\t"
>  "andl $3,%%ecx\n\t"
>  "jz 1f\n\t"
>  "rep ; movsb\n\t"
>  "1:"
>  : "=&c" (d0), "=&D" (d1), "=&S" (d2)
>  : "0" (n / 4), "g" (n), "1" ((long)to), "2"
> ((long)from)
>  : "memory");
> return to;
> }
> 
> I'd expected this function to use movl (32-bit moves) where possible, but
> movsb to get to naturally aligned moves (which is something that we deal
> with in the VME code already to use 16-bit reads where we can).
> 
> Greg (as co-maintainer of the VME subsystem :-) ), Am I reading this right?

Yes, but why would it matter if we copy by bytes, bits, or dwords to a
memory-backed bus?  By definition, that shouldn't matter.  If it does
matter, then we shouldn't be using memcpy, or relying on the compiler to
do the copying, but rather we need to use a write() function instead as
this would be IO memory that does care about this type of thing.

Does that help?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: Q: comedi timeouts

2014-02-05 Thread Hartley Sweeten 
On Wednesday, February 05, 2014 10:40 AM, Greg KH wrote:
> On Tue, Feb 04, 2014 at 06:59:43PM +, Hartley Sweeten wrote:
>> I'm in the process of cleaning up all the "timeout" code in the
>> comedi drivers.  The general form of the timeout code in all
>> the drivers is:
>> 
>> static int foo_eoc(struct comedi_device *dev, unsigned long timeout)
>> {
>>  unsigned int status;
>>  while (timeout--) {
>>  status = inb(dev->iobase + STATUS_REG);
>>  if (status & EOC)
>>  return 0;
>>  udelay(1);
>>  }
>>  return -ETIMEDOUT;
>> }
>> 
>> My question is about the 'udelay()'. Is this the preferred way to
>> delay between each check of the status register of would
>> something like a 'cpu_relax()' be better?
>
> udelay is fine, if you want really "tiny" sleep calls.  What about
> msleep() to really delay?
>
> And the whole "this is the number of loops" is just crazy, how about
> moving everything to use a real time value to timeout with instead?
> Then use the proper jiffies comparison functions so it's obvious as to
> what is being done here.

I agree that the "number of loops" stuff is nuts.

Some of the timeout loops are used in interrupt contexts. Can jiffies
comparisons be done then?

>> Also, the 'timeout' values used in the comedi drivers vary a lot.
>> I have seen value of 3, 5, 10, 100, 1000, 1, 4, and 10.
>> Usually the smaller values have the udelay(1) in the loop, the
>> larger values don't. Assuming cpu_relax() would be a better way
>> to handle the delays, would it be a good idea to standardize the
>> timeout wait to a set number of loops?
>
> Not number of loops, but real time values, that's easier to audit for
> and understand.

I agree. I'm just trying to figure out the "best" way to handle this.

I think a 1 second timeout should be more than enough time for any
analog input/output conversion to complete. If would also be nice
if the comedi drivers released the cpu for other processes while waiting
for the end-of-conversion. Based on that I was thinking about adding
a comedi core helper like this:

int comedi_eoc_timeout(struct comedi_device *dev,
   struct comedi_subdevice *s,
   struct comedi_insn *insn,
   int (*cb)(struct comedi_device *dev,
 struct comedi_subdevice *s,
 struct comedi_insn *insn))
{
unsigned long timeout = jiffies + msec_to_jiffies(1000);
int ret;

while (time_before(jiffies, timeout)) {
ret = cb(dev, s, insn);
if (ret == 0)
return 0;
if (ret != -EBUSY)
return ret;
cpu_relax();/* or udelay(1)? */
}
return -ETIMEDOUT;
}

Most of the drivers just need the 'dev' parameter to do the
checking but some need the 's' parameter to determine the
proper address to read. Some also need the channel that is
packed in the 'insn' to determine the bit to check.

The drivers would then just provide the callback that actually checks
for the end-of-conversion:

static int foo_eoc(struct comedi_device *dev,
   struct comedi_subdevice *s,
   struct comedi_insn *insn)
{
unsigned int status;

status = inl(dev->iobase FOO_STATUS_REG);
if (status & FOO_EOC_BIT)
return 0;
/*
 * Any other driver specific tests.
* All non 0 or -EBUSY errno's are propagated.
 */
return -EBUSY;
}

Then use core helper to wait for the end-of-conversion:

ret = comedi_eoc_timeout(dev, s, insn, fooc_eoc);
if (ret)
return ret;

This seems like it would cover all the comedi drivers. I also
gets all the timeout stuff into a common place.

Thanks,
Hartley

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Q: comedi timeouts

2014-02-05 Thread Greg KH 
On Tue, Feb 04, 2014 at 06:59:43PM +, Hartley Sweeten wrote:
> Hello all,
> 
> I'm in the process of cleaning up all the "timeout" code in the
> comedi drivers.  The general form of the timeout code in all
> the drivers is:
> 
> static int foo_eoc(struct comedi_device *dev, unsigned long timeout)
> {
>   unsigned int status;
>   while (timeout--) {
>   status = inb(dev->iobase + STATUS_REG);
>   if (status & EOC)
>   return 0;
>   udelay(1);
>   }
>   return -ETIMEDOUT;
> }
> 
> My question is about the 'udelay()'. Is this the preferred way to
> delay between each check of the status register of would
> something like a 'cpu_relax()' be better?

udelay is fine, if you want really "tiny" sleep calls.  What about
msleep() to really delay?

And the whole "this is the number of loops" is just crazy, how about
moving everything to use a real time value to timeout with instead?
Then use the proper jiffies comparison functions so it's obvious as to
what is being done here.

> Also, the 'timeout' values used in the comedi drivers vary a lot.
> I have seen value of 3, 5, 10, 100, 1000, 1, 4, and 10.
> Usually the smaller values have the udelay(1) in the loop, the
> larger values don't. Assuming cpu_relax() would be a better way
> to handle the delays, would it be a good idea to standardize the
> timeout wait to a set number of loops?

Not number of loops, but real time values, that's easier to audit for
and understand.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Q: comedi timeouts

2014-02-05 Thread Greg KH 
On Wed, Feb 05, 2014 at 06:02:04PM +, Hartley Sweeten wrote:
> On Wednesday, February 05, 2014 10:40 AM, Greg KH wrote:
> > On Tue, Feb 04, 2014 at 06:59:43PM +, Hartley Sweeten wrote:
> >> I'm in the process of cleaning up all the "timeout" code in the
> >> comedi drivers.  The general form of the timeout code in all
> >> the drivers is:
> >> 
> >> static int foo_eoc(struct comedi_device *dev, unsigned long timeout)
> >> {
> >>unsigned int status;
> >>while (timeout--) {
> >>status = inb(dev->iobase + STATUS_REG);
> >>if (status & EOC)
> >>return 0;
> >>udelay(1);
> >>}
> >>return -ETIMEDOUT;
> >> }
> >> 
> >> My question is about the 'udelay()'. Is this the preferred way to
> >> delay between each check of the status register of would
> >> something like a 'cpu_relax()' be better?
> >
> > udelay is fine, if you want really "tiny" sleep calls.  What about
> > msleep() to really delay?
> >
> > And the whole "this is the number of loops" is just crazy, how about
> > moving everything to use a real time value to timeout with instead?
> > Then use the proper jiffies comparison functions so it's obvious as to
> > what is being done here.
> 
> I agree that the "number of loops" stuff is nuts.
> 
> Some of the timeout loops are used in interrupt contexts. Can jiffies
> comparisons be done then?

Yes.

But if you are in interrupt context, you can't call cpu_relax().

> >> Also, the 'timeout' values used in the comedi drivers vary a lot.
> >> I have seen value of 3, 5, 10, 100, 1000, 1, 4, and 10.
> >> Usually the smaller values have the udelay(1) in the loop, the
> >> larger values don't. Assuming cpu_relax() would be a better way
> >> to handle the delays, would it be a good idea to standardize the
> >> timeout wait to a set number of loops?
> >
> > Not number of loops, but real time values, that's easier to audit for
> > and understand.
> 
> I agree. I'm just trying to figure out the "best" way to handle this.
> 
> I think a 1 second timeout should be more than enough time for any
> analog input/output conversion to complete. If would also be nice
> if the comedi drivers released the cpu for other processes while waiting
> for the end-of-conversion. Based on that I was thinking about adding
> a comedi core helper like this:
> 
> int comedi_eoc_timeout(struct comedi_device *dev,
>  struct comedi_subdevice *s,
>  struct comedi_insn *insn,
>  int (*cb)(struct comedi_device *dev,
>struct comedi_subdevice *s,
>struct comedi_insn *insn))
> {
>   unsigned long timeout = jiffies + msec_to_jiffies(1000);
>   int ret;
> 
>   while (time_before(jiffies, timeout)) {
>   ret = cb(dev, s, insn);
>   if (ret == 0)
>   return 0;
>   if (ret != -EBUSY)
>   return ret;
>   cpu_relax();/* or udelay(1)? */
>   }
>   return -ETIMEDOUT;
> }

Given this is such a common "pattern" in driver development, shouldn't
there be something like it in the driver core perhaps?

> Most of the drivers just need the 'dev' parameter to do the
> checking but some need the 's' parameter to determine the
> proper address to read. Some also need the channel that is
> packed in the 'insn' to determine the bit to check.
> 
> The drivers would then just provide the callback that actually checks
> for the end-of-conversion:
> 
> static int foo_eoc(struct comedi_device *dev,
>  struct comedi_subdevice *s,
>  struct comedi_insn *insn)
> {
>   unsigned int status;
> 
>   status = inl(dev->iobase FOO_STATUS_REG);
>   if (status & FOO_EOC_BIT)
>   return 0;
>   /*
>* Any other driver specific tests.
>   * All non 0 or -EBUSY errno's are propagated.
>*/
>   return -EBUSY;
> }
> 
> Then use core helper to wait for the end-of-conversion:
> 
>   ret = comedi_eoc_timeout(dev, s, insn, fooc_eoc);
>   if (ret)
>   return ret;
> 
> This seems like it would cover all the comedi drivers. I also
> gets all the timeout stuff into a common place.

Putting it in a common place would be great to have, if the driver core
can't handle it, then in the comedi core at the least sounds great.

But again, watch out for sleeping while in interrupt context, that's not
good.

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/8] Fix some bugs/races in imx-drm

2014-02-05 Thread Greg Kroah-Hartman 
On Tue, Dec 17, 2013 at 05:13:48PM -0800, Greg Kroah-Hartman wrote:
> On Tue, Dec 17, 2013 at 07:09:56PM +, Russell King - ARM Linux wrote:
> > On Tue, Dec 17, 2013 at 08:05:41AM -0800, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 17, 2013 at 03:17:02PM +, Russell King - ARM Linux wrote:
> > > > On Tue, Dec 17, 2013 at 01:04:20PM +0800, Shawn Guo wrote:
> > > > > On Mon, Dec 16, 2013 at 12:38:23PM +, Russell King - ARM Linux 
> > > > > wrote:
> > > > > > Russell King (8):
> > > > > >   imx-drm: imx-drm-core: fix error cleanup path for 
> > > > > > imx_drm_add_crtc()
> > > > > >   imx-drm: imx-drm-core: fix DRM cleanup paths
> > > > > >   imx-drm: fix missing symbol exports
> > > > > 
> > > > > Except the little doubt I replied on this one, for the whole series:
> > > > > 
> > > > > Acked-by: Shawn Guo 
> > > > > Tested-by: Shawn Guo 
> > > > > 
> > > > > >   imx-drm: ipu-v3: fix potential CRTC device registration race
> > > > > >   imx-drm: imx-tve: don't call sleeping functions beneath 
> > > > > > enable_lock spinlo
> > > > > >   imx-drm: imx-drm-core: use defined constant for number of 
> > > > > > CRTCs.
> > > > > >   imx-drm: imx-drm-core: make imx_drm_crtc_register() safer
> > > > > >   imx-drm: imx-drm-core: improve safety of imx_drm_add_crtc()
> > > > 
> > > > Greg,
> > > > 
> > > > Do you want me to re-send them with these acks/tested-by's added, or are
> > > > you happy to take them as-is?
> > > 
> > > I can take these as-is.
> > 
> > Thanks.
> > 
> > > > I also have some further patches to send (I think three) which apply on
> > > > top of these but are not fixes (so aren't -rc material), more cleanups.
> > > 
> > > Ok, feel free to send them, I can take them.
> > 
> > They'll be along shortly.  Diffstat looks smaller - just one file:
> > 
> >  drivers/staging/imx-drm/imx-drm-core.c |   55 
> > +++
> >  1 files changed, 20 insertions(+), 35 deletions(-)
> > 
> > Russell King (3):
> >   imx-drm: imx-drm-core: use the crtc drm device for vblank
> >   imx-drm: imx-drm-core: avoid going the long route round for drm_device
> >   imx-drm: imx-drm-core: merge imx_drm_crtc_register() into 
> > imx_drm_add_crtc()
> 
> I'll apply these after the first round goes into Linus's tree to make
> the merging easier (i.e. after the next -rc).

Now applied.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] imx-drm: imx-hdmi: Remove parentheses from returns

2014-02-05 Thread Fabio Estevam 
From: Fabio Estevam 

Fix the following checkpatch warnings:

ERROR: return is not a function, parentheses are not required
#462: FILE: drivers/staging/imx-drm/imx-hdmi.c:462:
+   return (hdmi->hdmi_data.enc_in_format !=

ERROR: return is not a function, parentheses are not required
#468: FILE: drivers/staging/imx-drm/imx-hdmi.c:468:
+   return ((hdmi->hdmi_data.enc_out_format == YCBCR422_8BITS) &&

ERROR: return is not a function, parentheses are not required
#475: FILE: drivers/staging/imx-drm/imx-hdmi.c:475:
+   return ((hdmi->hdmi_data.enc_in_format == YCBCR422_8BITS) &&

Signed-off-by: Fabio Estevam 
---
 drivers/staging/imx-drm/imx-hdmi.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-hdmi.c 
b/drivers/staging/imx-drm/imx-hdmi.c
index 62ce0e8..8c15e07 100644
--- a/drivers/staging/imx-drm/imx-hdmi.c
+++ b/drivers/staging/imx-drm/imx-hdmi.c
@@ -459,22 +459,22 @@ static void hdmi_video_sample(struct imx_hdmi *hdmi)
 
 static int is_color_space_conversion(struct imx_hdmi *hdmi)
 {
-   return (hdmi->hdmi_data.enc_in_format !=
-   hdmi->hdmi_data.enc_out_format);
+   return hdmi->hdmi_data.enc_in_format !=
+   hdmi->hdmi_data.enc_out_format;
 }
 
 static int is_color_space_decimation(struct imx_hdmi *hdmi)
 {
-   return ((hdmi->hdmi_data.enc_out_format == YCBCR422_8BITS) &&
+   return (hdmi->hdmi_data.enc_out_format == YCBCR422_8BITS) &&
(hdmi->hdmi_data.enc_in_format == RGB ||
-   hdmi->hdmi_data.enc_in_format == YCBCR444));
+   hdmi->hdmi_data.enc_in_format == YCBCR444);
 }
 
 static int is_color_space_interpolation(struct imx_hdmi *hdmi)
 {
-   return ((hdmi->hdmi_data.enc_in_format == YCBCR422_8BITS) &&
+   return (hdmi->hdmi_data.enc_in_format == YCBCR422_8BITS) &&
(hdmi->hdmi_data.enc_out_format == RGB ||
-   hdmi->hdmi_data.enc_out_format == YCBCR444));
+   hdmi->hdmi_data.enc_out_format == YCBCR444);
 }
 
 static void imx_hdmi_update_csc_coeffs(struct imx_hdmi *hdmi)
-- 
1.8.1.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: vme_tsi148 question

2014-02-05 Thread Michael Kenney 
On Wed, Feb 5, 2014 at 1:22 PM, Martyn Welch  wrote:
>
>
>
> On 5 February 2014 17:41, Greg KH  wrote:
>>
>> On Tue, Feb 04, 2014 at 06:34:13PM +, Martyn Welch wrote:
>> > On 04/02/14 16:34, Michael Kenney wrote:
>> > >Hi Martyn,
>> > >
>> > >On Tue, Feb 4, 2014 at 7:28 AM, Martyn Welch > > >> wrote:
>> > >
>> > >On 28/12/13 00:34, Michael Kenney wrote:
>> > >
>> > >Hi Martyn,
>> > >
>> > >On Fri, Dec 27, 2013 at 4:23 PM, Martyn Welch
>> > >mailto:mar...@welchs.me.uk>> wrote:
>> > >
>> > >On 27/12/13 20:15, Michael Kenney wrote:
>> > >
>> > >
>> > >We are using the vme_tsi148 bridge driver along with
>> > > the
>> > >vme_user
>> > >driver to access the VME boards. The A/D board requires
>> > >D32 bus cycles
>> > >and the VME master window is configured accordingly,
>> > >however, when
>> > >monitoring the bus cycles with a logic analyzer, we
>> > >noticed that the
>> > >CPU is transferring one byte at a time (i.e. four D8
>> > >transfers rather
>> > >than one D32).
>> > >
>> > >Is this the expected behavior of the tsi148 driver?
>> > >
>> > >
>> > >Hi Mike,
>> > >
>> > >This is certainly not the expected behaviour - if the
>> > > window
>> > >is configured
>> > >for D32 then it should do 32 bit transfers where possible.
>> > >
>> > >I've heard of this happening recently, but haven't yet been
>> > >able to
>> > >replicate it. Which VME board are you running Linux on and
>> > >which flavour of
>> > >Linux?
>> > >
>> > >
>> > >I'm running Debian 7.2 with kernel 3.2 on a Fastwel CPC600
>> > >(Pentium M
>> > >based CPU board).
>> > >
>> > >
>> > >I haven't forgotten about this, still not sure exactly what is
>> > >happening.
>> > >
>> > >Is your install/kernel 32 or 64 bit?
>> > >
>> > >Are you doing single 32-bit transfers, or are you seeing this on
>> > >longer transfers (i.e. copying a buffer full of data)?
>> > >
>> > >
>> > >Thanks for getting back to me.
>> > >
>> > >I'm running a 32-bit kernel and I see this behavior on all transfers
>> > >regardless of buffer size.
>> > >
>> >
>> > Gah! Thought I could see what may be causing it in 64-bit kernels, but
>> > not
>> > 32-bit (my x86 asm is not particularly hot).
>> >
>> > I think we /may/ be hitting issues with how the memcpy function gets
>> > implemented on specific architectures. The tsi148 is a PCI/X to VME
>> > bridge,
>> > if it receives a series of 8-bit reads or writes, it translates these to
>> > 8-bit reads or writes on the VME bus, which is not necessarily what we
>> > want.
>> >
>> > According to the data sheet, the card you are using has an Intel 6300ESB
>> > ICH, which seems to be PCI/X (so we can rule out PCIe to PCI/X bridges
>> > or
>> > something like that doing nasty things).
>> >
>> > I think (if I follow everything correctly) then the memcpy for 32-bit is
>> > handled by __memcpy in arch/x86/include/asm/string_32.h:
>> >
>> > static __always_inline void *__memcpy(void *to, const void *from, size_t
>> > n)
>> > {
>> > int d0, d1, d2;
>> > asm volatile("rep ; movsl\n\t"
>> >  "movl %4,%%ecx\n\t"
>> >  "andl $3,%%ecx\n\t"
>> >  "jz 1f\n\t"
>> >  "rep ; movsb\n\t"
>> >  "1:"
>> >  : "=&c" (d0), "=&D" (d1), "=&S" (d2)
>> >  : "0" (n / 4), "g" (n), "1" ((long)to), "2"
>> > ((long)from)
>> >  : "memory");
>> > return to;
>> > }
>> >
>> > I'd expected this function to use movl (32-bit moves) where possible,
>> > but
>> > movsb to get to naturally aligned moves (which is something that we deal
>> > with in the VME code already to use 16-bit reads where we can).
>> >
>> > Greg (as co-maintainer of the VME subsystem :-) ), Am I reading this
>> > right?
>>
>> Yes, but why would it matter if we copy by bytes, bits, or dwords to a
>> memory-backed bus?  By definition, that shouldn't matter.  If it does
>> matter, then we shouldn't be using memcpy, or relying on the compiler to
>> do the copying, but rather we need to use a write() function instead as
>> this would be IO memory that does care about this type of thing.
>>
>
> The drivers are using the functions memcpy_toio and memcpy_fromio for the
> bulk of the transfer, these seem to equate to memcpy on x86.
>
> In this instance it does matter - some devices can only accept certain width
> accesses and the VME bridge will translate a 8-bit PCI transfer to a 8-bit
> transfer on the VME bus.
>
> So (and I haven't even compile tested this yet), this?:
>
> diff --git a/drivers/vme/bridges/vme_ca91cx42.c
> b/drivers

[PATCH v2] staging: comedi: adv_pci1710: fix analog output readback value

2014-02-05 Thread H Hartley Sweeten 
The last value written to a analog output channel is cached in the
private data of this driver for readback.

Currently, the wrong value is cached in the (*insn_write) functions.
The current code stores the data[n] value for readback afer the loop
has written all the values. At this time 'n' points past the end of
the data array.

Fix the functions by using a local variable to hold the data being
written to the analog output channel. This variable is then used
after the loop is complete to store the readback value. The current
value is retrieved before the loop in case no values are actually
written..

Signed-off-by: H Hartley Sweeten 
Cc: Ian Abbott 
Cc: Greg Kroah-Hartman 
---

v2: also fix readback in pci1720_insn_write_ao() as pointed out by
Ian Abbott

 drivers/staging/comedi/drivers/adv_pci1710.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c 
b/drivers/staging/comedi/drivers/adv_pci1710.c
index 69a8298..f9d6111 100644
--- a/drivers/staging/comedi/drivers/adv_pci1710.c
+++ b/drivers/staging/comedi/drivers/adv_pci1710.c
@@ -494,6 +494,7 @@ static int pci171x_insn_write_ao(struct comedi_device *dev,
 struct comedi_insn *insn, unsigned int *data)
 {
struct pci1710_private *devpriv = dev->private;
+   unsigned int val;
int n, chan, range, ofs;
 
chan = CR_CHAN(insn->chanspec);
@@ -509,11 +510,14 @@ static int pci171x_insn_write_ao(struct comedi_device 
*dev,
outw(devpriv->da_ranges, dev->iobase + PCI171x_DAREF);
ofs = PCI171x_DA1;
}
+   val = devpriv->ao_data[chan];
 
-   for (n = 0; n < insn->n; n++)
-   outw(data[n], dev->iobase + ofs);
+   for (n = 0; n < insn->n; n++) {
+   val = data[n];
+   outw(val, dev->iobase + ofs);
+   }
 
-   devpriv->ao_data[chan] = data[n];
+   devpriv->ao_data[chan] = val;
 
return n;
 
@@ -679,6 +683,7 @@ static int pci1720_insn_write_ao(struct comedi_device *dev,
 struct comedi_insn *insn, unsigned int *data)
 {
struct pci1710_private *devpriv = dev->private;
+   unsigned int val;
int n, rangereg, chan;
 
chan = CR_CHAN(insn->chanspec);
@@ -688,13 +693,15 @@ static int pci1720_insn_write_ao(struct comedi_device 
*dev,
outb(rangereg, dev->iobase + PCI1720_RANGE);
devpriv->da_ranges = rangereg;
}
+   val = devpriv->ao_data[chan];
 
for (n = 0; n < insn->n; n++) {
-   outw(data[n], dev->iobase + PCI1720_DA0 + (chan << 1));
+   val = data[n];
+   outw(val, dev->iobase + PCI1720_DA0 + (chan << 1));
outb(0, dev->iobase + PCI1720_SYNCOUT); /*  update outputs */
}
 
-   devpriv->ao_data[chan] = data[n];
+   devpriv->ao_data[chan] = val;
 
return n;
 }
-- 
1.8.5.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/8] Fix some bugs/races in imx-drm

2014-02-05 Thread Russell King - ARM Linux 
On Wed, Feb 05, 2014 at 12:02:49PM -0800, Greg Kroah-Hartman wrote:
> On Tue, Dec 17, 2013 at 05:13:48PM -0800, Greg Kroah-Hartman wrote:
> > I'll apply these after the first round goes into Linus's tree to make
> > the merging easier (i.e. after the next -rc).
> 
> Now applied.

Thanks.  I'll see about sending the next set out for review - hopefully
I can get all of them out for the next merge window.

I do have one minor fix for the component stuff you merged which I'll
send out in the next few days (it's just to ensure that devm resources
for the master device are properly handled.)  And thanks for merging
that during the previous merge window, that has been a great help.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: vme_tsi148 question

2014-02-05 Thread Michael Kenney 
On Wed, Feb 5, 2014 at 1:38 PM, Michael Kenney  wrote:
> On Wed, Feb 5, 2014 at 1:22 PM, Martyn Welch  wrote:
>>
>>
>>
>> On 5 February 2014 17:41, Greg KH  wrote:
>>>
>>> On Tue, Feb 04, 2014 at 06:34:13PM +, Martyn Welch wrote:
>>> > On 04/02/14 16:34, Michael Kenney wrote:
>>> > >Hi Martyn,
>>> > >
>>> > >On Tue, Feb 4, 2014 at 7:28 AM, Martyn Welch >> > >> wrote:
>>> > >
>>> > >On 28/12/13 00:34, Michael Kenney wrote:
>>> > >
>>> > >Hi Martyn,
>>> > >
>>> > >On Fri, Dec 27, 2013 at 4:23 PM, Martyn Welch
>>> > >mailto:mar...@welchs.me.uk>> wrote:
>>> > >
>>> > >On 27/12/13 20:15, Michael Kenney wrote:
>>> > >
>>> > >
>>> > >We are using the vme_tsi148 bridge driver along with
>>> > > the
>>> > >vme_user
>>> > >driver to access the VME boards. The A/D board requires
>>> > >D32 bus cycles
>>> > >and the VME master window is configured accordingly,
>>> > >however, when
>>> > >monitoring the bus cycles with a logic analyzer, we
>>> > >noticed that the
>>> > >CPU is transferring one byte at a time (i.e. four D8
>>> > >transfers rather
>>> > >than one D32).
>>> > >
>>> > >Is this the expected behavior of the tsi148 driver?
>>> > >
>>> > >
>>> > >Hi Mike,
>>> > >
>>> > >This is certainly not the expected behaviour - if the
>>> > > window
>>> > >is configured
>>> > >for D32 then it should do 32 bit transfers where possible.
>>> > >
>>> > >I've heard of this happening recently, but haven't yet been
>>> > >able to
>>> > >replicate it. Which VME board are you running Linux on and
>>> > >which flavour of
>>> > >Linux?
>>> > >
>>> > >
>>> > >I'm running Debian 7.2 with kernel 3.2 on a Fastwel CPC600
>>> > >(Pentium M
>>> > >based CPU board).
>>> > >
>>> > >
>>> > >I haven't forgotten about this, still not sure exactly what is
>>> > >happening.
>>> > >
>>> > >Is your install/kernel 32 or 64 bit?
>>> > >
>>> > >Are you doing single 32-bit transfers, or are you seeing this on
>>> > >longer transfers (i.e. copying a buffer full of data)?
>>> > >
>>> > >
>>> > >Thanks for getting back to me.
>>> > >
>>> > >I'm running a 32-bit kernel and I see this behavior on all transfers
>>> > >regardless of buffer size.
>>> > >
>>> >
>>> > Gah! Thought I could see what may be causing it in 64-bit kernels, but
>>> > not
>>> > 32-bit (my x86 asm is not particularly hot).
>>> >
>>> > I think we /may/ be hitting issues with how the memcpy function gets
>>> > implemented on specific architectures. The tsi148 is a PCI/X to VME
>>> > bridge,
>>> > if it receives a series of 8-bit reads or writes, it translates these to
>>> > 8-bit reads or writes on the VME bus, which is not necessarily what we
>>> > want.
>>> >
>>> > According to the data sheet, the card you are using has an Intel 6300ESB
>>> > ICH, which seems to be PCI/X (so we can rule out PCIe to PCI/X bridges
>>> > or
>>> > something like that doing nasty things).
>>> >
>>> > I think (if I follow everything correctly) then the memcpy for 32-bit is
>>> > handled by __memcpy in arch/x86/include/asm/string_32.h:
>>> >
>>> > static __always_inline void *__memcpy(void *to, const void *from, size_t
>>> > n)
>>> > {
>>> > int d0, d1, d2;
>>> > asm volatile("rep ; movsl\n\t"
>>> >  "movl %4,%%ecx\n\t"
>>> >  "andl $3,%%ecx\n\t"
>>> >  "jz 1f\n\t"
>>> >  "rep ; movsb\n\t"
>>> >  "1:"
>>> >  : "=&c" (d0), "=&D" (d1), "=&S" (d2)
>>> >  : "0" (n / 4), "g" (n), "1" ((long)to), "2"
>>> > ((long)from)
>>> >  : "memory");
>>> > return to;
>>> > }
>>> >
>>> > I'd expected this function to use movl (32-bit moves) where possible,
>>> > but
>>> > movsb to get to naturally aligned moves (which is something that we deal
>>> > with in the VME code already to use 16-bit reads where we can).
>>> >
>>> > Greg (as co-maintainer of the VME subsystem :-) ), Am I reading this
>>> > right?
>>>
>>> Yes, but why would it matter if we copy by bytes, bits, or dwords to a
>>> memory-backed bus?  By definition, that shouldn't matter.  If it does
>>> matter, then we shouldn't be using memcpy, or relying on the compiler to
>>> do the copying, but rather we need to use a write() function instead as
>>> this would be IO memory that does care about this type of thing.
>>>
>>
>> The drivers are using the functions memcpy_toio and memcpy_fromio for the
>> bulk of the transfer, these seem to equate to memcpy on x86.
>>
>> In this instance it does matter - some devices can only accept certain width
>> accesses and the VME bridge will transla

[PATCH] staging: gdm72xx: fix leaks at failure path in gdm_usb_probe()

2014-02-05 Thread Alexey Khoroshilov 
Error handling code in gdm_usb_probe() misses to deallocate
tx_ and rx_structs and to do usb_put_dev().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov 
---
 drivers/staging/gdm72xx/gdm_usb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/gdm72xx/gdm_usb.c 
b/drivers/staging/gdm72xx/gdm_usb.c
index f8788bf0a7d3..cdeffe75496b 100644
--- a/drivers/staging/gdm72xx/gdm_usb.c
+++ b/drivers/staging/gdm72xx/gdm_usb.c
@@ -635,11 +635,14 @@ static int gdm_usb_probe(struct usb_interface *intf,
 #endif /* CONFIG_WIMAX_GDM72XX_USB_PM */
 
ret = register_wimax_device(phy_dev, &intf->dev);
+   if (ret)
+   release_usb(udev);
 
 out:
if (ret) {
kfree(phy_dev);
kfree(udev);
+   usb_put_dev(usbdev);
} else {
usb_set_intfdata(intf, phy_dev);
}
-- 
1.8.3.2

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel