Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread David Miller
From: Joe Perches 
Date: Fri, 21 Jun 2013 07:43:33 -0700

> Maybe add a Kconfig "depends on !64BIT".

Please, no.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


RE: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread David Laight
> So it should be declared dma_addr_t then,
> 
> > +   addr = dma_map_single(>dev, (void 
> > *)rx_buff->skb->data,
> > + buflen, DMA_FROM_DEVICE);
> > +   if (dma_mapping_error(>dev, addr)) {
> > +   if (net_ratelimit())
> > +   netdev_err(ndev, "cannot dma map\n");
> > +   dev_kfree_skb(rx_buff->skb);
> > +   stats->rx_errors++;
> > +   continue;
> > +   }
> > +   dma_unmap_addr_set(_buff, mapping, addr);
> > +   dma_unmap_len_set(_buff, len, buflen);
> > +
> > +   rxbd->data = (unsigned char 
> > *)cpu_to_le32(rx_buff->skb->data);
> >
> > the 'addr' returned by dma_map_single is what the device really
> > needs, although it is the same as rx_buff->skb->data with the
> > trivial definition of dma_map_single.
> >
> > The last line here needs to be
> > rxbd->data = cpu_to_le32(addr);
> >
> > which fixes the bug, and has no dependency on a 32 bit CPU.
> 
> It still has a dependency on dma_addr_t size being 32 bit

No - just a dependency on memory being mapped to a 32bit
physical address accessible from the device.
Many 64 bit systems have devices that can only access 32bit
address space, either the memory has to be allocated from the
correct arena, of DMA bounce buffers are used.

David



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Joe Perches
On Fri, 2013-06-21 at 17:58 +0200, Arnd Bergmann wrote:
> On Friday 21 June 2013, Joe Perches wrote:
> > On Fri, 2013-06-21 at 10:53 +, Alexey Brodkin wrote:
> > > On 06/21/2013 02:32 PM, Joe Perches wrote:
> > > > On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
> > > >> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> > > >> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> > > >> ARCAngel4/ML50x.
> > []
> > > >> +  rxbd->data = (unsigned char 
> > > >> *)cpu_to_le32(rx_buff->skb->data);
> > > >
> > > > 32 bit only.  Should the Kconfig block have some arch_arc dependency
> > > > so it can't get compiled for 64 bit systems?
> > []
> > > So for now I may easily add dependency on ARC if it makes acceptance of 
> > > driver easier.
> > 
> > I don't think it's a big problem.
> > 
> > Maybe add a Kconfig "depends on !64BIT".
> > 
> > Another thing to do would be to run it through sparse
> > with make C=1
> 
> No, I think the driver should just be made 64-bit clean. Not because
> anyone is going to need that of course, but to avoid having
> to add silly dependencies.
> 
> The problem is that rxbd->data is the wrong type. It is declared
> as a void*, but it is not a kernel pointer at all, it is a DMA
> pointer. Look at this code in context:

So it should be declared dma_addr_t then,

> +   addr = dma_map_single(>dev, (void *)rx_buff->skb->data,
> + buflen, DMA_FROM_DEVICE);
> +   if (dma_mapping_error(>dev, addr)) {
> +   if (net_ratelimit())
> +   netdev_err(ndev, "cannot dma map\n");
> +   dev_kfree_skb(rx_buff->skb);
> +   stats->rx_errors++;
> +   continue;
> +   }
> +   dma_unmap_addr_set(_buff, mapping, addr);
> +   dma_unmap_len_set(_buff, len, buflen);
> +
> +   rxbd->data = (unsigned char *)cpu_to_le32(rx_buff->skb->data);
> 
> the 'addr' returned by dma_map_single is what the device really
> needs, although it is the same as rx_buff->skb->data with the
> trivial definition of dma_map_single.
> 
> The last line here needs to be
> rxbd->data = cpu_to_le32(addr);
> 
> which fixes the bug, and has no dependency on a 32 bit CPU.

It still has a dependency on dma_addr_t size being 32 bit



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Arnd Bergmann
On Friday 21 June 2013, Joe Perches wrote:
> On Fri, 2013-06-21 at 10:53 +, Alexey Brodkin wrote:
> > On 06/21/2013 02:32 PM, Joe Perches wrote:
> > > On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
> > >> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> > >> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> > >> ARCAngel4/ML50x.
> []
> > >> +  rxbd->data = (unsigned char *)cpu_to_le32(rx_buff->skb->data);
> > >
> > > 32 bit only.  Should the Kconfig block have some arch_arc dependency
> > > so it can't get compiled for 64 bit systems?
> []
> > So for now I may easily add dependency on ARC if it makes acceptance of 
> > driver easier.
> 
> I don't think it's a big problem.
> 
> Maybe add a Kconfig "depends on !64BIT".
> 
> Another thing to do would be to run it through sparse
> with make C=1

No, I think the driver should just be made 64-bit clean. Not because
anyone is going to need that of course, but to avoid having
to add silly dependencies.

The problem is that rxbd->data is the wrong type. It is declared
as a void*, but it is not a kernel pointer at all, it is a DMA
pointer. Look at this code in context:


+   addr = dma_map_single(>dev, (void *)rx_buff->skb->data,
+ buflen, DMA_FROM_DEVICE);
+   if (dma_mapping_error(>dev, addr)) {
+   if (net_ratelimit())
+   netdev_err(ndev, "cannot dma map\n");
+   dev_kfree_skb(rx_buff->skb);
+   stats->rx_errors++;
+   continue;
+   }
+   dma_unmap_addr_set(_buff, mapping, addr);
+   dma_unmap_len_set(_buff, len, buflen);
+
+   rxbd->data = (unsigned char *)cpu_to_le32(rx_buff->skb->data);

the 'addr' returned by dma_map_single is what the device really
needs, although it is the same as rx_buff->skb->data with the
trivial definition of dma_map_single.

The last line here needs to be

rxbd->data = cpu_to_le32(addr);

which fixes the bug, and has no dependency on a 32 bit CPU.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Joe Perches
On Fri, 2013-06-21 at 10:53 +, Alexey Brodkin wrote:
> On 06/21/2013 02:32 PM, Joe Perches wrote:
> > On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
> >> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> >> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> >> ARCAngel4/ML50x.
[]
> >> +  rxbd->data = (unsigned char *)cpu_to_le32(rx_buff->skb->data);
> >
> > 32 bit only.  Should the Kconfig block have some arch_arc dependency
> > so it can't get compiled for 64 bit systems?
[]
> So for now I may easily add dependency on ARC if it makes acceptance of 
> driver easier.

I don't think it's a big problem.

Maybe add a Kconfig "depends on !64BIT".

Another thing to do would be to run it through sparse
with make C=1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Andy Shevchenko
On Fri, Jun 21, 2013 at 3:25 PM, Alexey Brodkin
 wrote:
> Any other comments on patch itself?

Looks fine to me.

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Alexey Brodkin
On 06/21/2013 04:15 PM, Andy Shevchenko wrote:
> On Fri, Jun 21, 2013 at 1:53 PM, Alexey Brodkin
>  wrote:
>
> []
>
>> But during compilation on x86 one needs to enable OF. Which will lead to
>> compilation errors in other things.
>
> JFYI, x86 has at least one platform with official OF dependency.
> Once I actually tried to build kernel for Intel MID with OF enabled
> (it required a hack in the arch Kconfig, of course), compiled fine.

Ok, good to know it.

Any other comments on patch itself?

-Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Andy Shevchenko
On Fri, Jun 21, 2013 at 1:53 PM, Alexey Brodkin
 wrote:

[]

> But during compilation on x86 one needs to enable OF. Which will lead to
> compilation errors in other things.

JFYI, x86 has at least one platform with official OF dependency.
Once I actually tried to build kernel for Intel MID with OF enabled
(it required a hack in the arch Kconfig, of course), compiled fine.

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Alexey Brodkin
On 06/21/2013 02:32 PM, Joe Perches wrote:
> On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
>> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
>> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
>> ARCAngel4/ML50x.
>
> Looks fine to me.
>
> One nit you could fix later and a question.
>
>> diff --git a/drivers/net/ethernet/arc/emac_main.c 
>> b/drivers/net/ethernet/arc/emac_main.c
> []
>> +static int arc_emac_rx(struct net_device *ndev, int budget)
>> +{
> []
>> +if (net_ratelimit())
>> +netdev_err(ndev, "incomplete packed 
>> received\n");
>
> s/packed/packet/

Ooops. Thanks for noticing this subtle misspelling.

>> +rxbd->data = (unsigned char *)cpu_to_le32(rx_buff->skb->data);
>
> 32 bit only.  Should the Kconfig block have some arch_arc dependency
> so it can't get compiled for 64 bit systems?

Well, initially I add dependency on ARC in Kconfig.
But later removed it because in driver itself there're really no 
ARC-specific things. So why don't make it available for any other arch?
For example I re-used Xilinx SystemACE driver on ARC even though it was 
only targeted for PPC/Microblaze. And now I need to add another 
dependency (+ || ARC) in Kconfig for all block devices.

But your comment regarding 64-bit arches makes perfect sense. But still 
I don't know if there's a way to restrict usage on only 32-bit arches 
(independent of being x86/arc/mips/etc). I may suppose that only strict 
specification of all virtually supported 32-bit arches - which is 
nonsense I think.

Also independence of architecture makes it possible to compile driver on 
x86 for x86 with native toolchain. In other words anybody may try to 
compile it. And I thought it's if not mandatory but at least very 
welcome - if maintainer or reviewer may compile stuff and teach me how 
to eliminate all compile-time warnings/errors.

But during compilation on x86 one needs to enable OF. Which will lead to 
compilation errors in other things.

So for now I may easily add dependency on ARC if it makes acceptance of 
driver easier.

-Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Joe Perches
On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> ARCAngel4/ML50x.

Looks fine to me.

One nit you could fix later and a question.

> diff --git a/drivers/net/ethernet/arc/emac_main.c 
> b/drivers/net/ethernet/arc/emac_main.c
[]
> +static int arc_emac_rx(struct net_device *ndev, int budget)
> +{
[]
> + if (net_ratelimit())
> + netdev_err(ndev, "incomplete packed 
> received\n");

s/packed/packet/

> + rxbd->data = (unsigned char *)cpu_to_le32(rx_buff->skb->data);

32 bit only.  Should the Kconfig block have some arch_arc dependency
so it can't get compiled for 64 bit systems?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Joe Perches
On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
 Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
 instantiated in some legacy ARC (Synopsys) FPGA Boards such as
 ARCAngel4/ML50x.

Looks fine to me.

One nit you could fix later and a question.

 diff --git a/drivers/net/ethernet/arc/emac_main.c 
 b/drivers/net/ethernet/arc/emac_main.c
[]
 +static int arc_emac_rx(struct net_device *ndev, int budget)
 +{
[]
 + if (net_ratelimit())
 + netdev_err(ndev, incomplete packed 
 received\n);

s/packed/packet/

 + rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);

32 bit only.  Should the Kconfig block have some arch_arc dependency
so it can't get compiled for 64 bit systems?

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


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Alexey Brodkin
On 06/21/2013 02:32 PM, Joe Perches wrote:
 On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
 Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
 instantiated in some legacy ARC (Synopsys) FPGA Boards such as
 ARCAngel4/ML50x.

 Looks fine to me.

 One nit you could fix later and a question.

 diff --git a/drivers/net/ethernet/arc/emac_main.c 
 b/drivers/net/ethernet/arc/emac_main.c
 []
 +static int arc_emac_rx(struct net_device *ndev, int budget)
 +{
 []
 +if (net_ratelimit())
 +netdev_err(ndev, incomplete packed 
 received\n);

 s/packed/packet/

Ooops. Thanks for noticing this subtle misspelling.

 +rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);

 32 bit only.  Should the Kconfig block have some arch_arc dependency
 so it can't get compiled for 64 bit systems?

Well, initially I add dependency on ARC in Kconfig.
But later removed it because in driver itself there're really no 
ARC-specific things. So why don't make it available for any other arch?
For example I re-used Xilinx SystemACE driver on ARC even though it was 
only targeted for PPC/Microblaze. And now I need to add another 
dependency (+ || ARC) in Kconfig for all block devices.

But your comment regarding 64-bit arches makes perfect sense. But still 
I don't know if there's a way to restrict usage on only 32-bit arches 
(independent of being x86/arc/mips/etc). I may suppose that only strict 
specification of all virtually supported 32-bit arches - which is 
nonsense I think.

Also independence of architecture makes it possible to compile driver on 
x86 for x86 with native toolchain. In other words anybody may try to 
compile it. And I thought it's if not mandatory but at least very 
welcome - if maintainer or reviewer may compile stuff and teach me how 
to eliminate all compile-time warnings/errors.

But during compilation on x86 one needs to enable OF. Which will lead to 
compilation errors in other things.

So for now I may easily add dependency on ARC if it makes acceptance of 
driver easier.

-Alexey
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Andy Shevchenko
On Fri, Jun 21, 2013 at 1:53 PM, Alexey Brodkin
alexey.brod...@synopsys.com wrote:

[]

 But during compilation on x86 one needs to enable OF. Which will lead to
 compilation errors in other things.

JFYI, x86 has at least one platform with official OF dependency.
Once I actually tried to build kernel for Intel MID with OF enabled
(it required a hack in the arch Kconfig, of course), compiled fine.

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Alexey Brodkin
On 06/21/2013 04:15 PM, Andy Shevchenko wrote:
 On Fri, Jun 21, 2013 at 1:53 PM, Alexey Brodkin
 alexey.brod...@synopsys.com wrote:

 []

 But during compilation on x86 one needs to enable OF. Which will lead to
 compilation errors in other things.

 JFYI, x86 has at least one platform with official OF dependency.
 Once I actually tried to build kernel for Intel MID with OF enabled
 (it required a hack in the arch Kconfig, of course), compiled fine.

Ok, good to know it.

Any other comments on patch itself?

-Alexey
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Andy Shevchenko
On Fri, Jun 21, 2013 at 3:25 PM, Alexey Brodkin
alexey.brod...@synopsys.com wrote:
 Any other comments on patch itself?

Looks fine to me.

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Joe Perches
On Fri, 2013-06-21 at 10:53 +, Alexey Brodkin wrote:
 On 06/21/2013 02:32 PM, Joe Perches wrote:
  On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
  Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
  instantiated in some legacy ARC (Synopsys) FPGA Boards such as
  ARCAngel4/ML50x.
[]
  +  rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);
 
  32 bit only.  Should the Kconfig block have some arch_arc dependency
  so it can't get compiled for 64 bit systems?
[]
 So for now I may easily add dependency on ARC if it makes acceptance of 
 driver easier.

I don't think it's a big problem.

Maybe add a Kconfig depends on !64BIT.

Another thing to do would be to run it through sparse
with make C=1


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


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Arnd Bergmann
On Friday 21 June 2013, Joe Perches wrote:
 On Fri, 2013-06-21 at 10:53 +, Alexey Brodkin wrote:
  On 06/21/2013 02:32 PM, Joe Perches wrote:
   On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
   Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
   instantiated in some legacy ARC (Synopsys) FPGA Boards such as
   ARCAngel4/ML50x.
 []
   +  rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);
  
   32 bit only.  Should the Kconfig block have some arch_arc dependency
   so it can't get compiled for 64 bit systems?
 []
  So for now I may easily add dependency on ARC if it makes acceptance of 
  driver easier.
 
 I don't think it's a big problem.
 
 Maybe add a Kconfig depends on !64BIT.
 
 Another thing to do would be to run it through sparse
 with make C=1

No, I think the driver should just be made 64-bit clean. Not because
anyone is going to need that of course, but to avoid having
to add silly dependencies.

The problem is that rxbd-data is the wrong type. It is declared
as a void*, but it is not a kernel pointer at all, it is a DMA
pointer. Look at this code in context:


+   addr = dma_map_single(ndev-dev, (void *)rx_buff-skb-data,
+ buflen, DMA_FROM_DEVICE);
+   if (dma_mapping_error(ndev-dev, addr)) {
+   if (net_ratelimit())
+   netdev_err(ndev, cannot dma map\n);
+   dev_kfree_skb(rx_buff-skb);
+   stats-rx_errors++;
+   continue;
+   }
+   dma_unmap_addr_set(rx_buff, mapping, addr);
+   dma_unmap_len_set(rx_buff, len, buflen);
+
+   rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);

the 'addr' returned by dma_map_single is what the device really
needs, although it is the same as rx_buff-skb-data with the
trivial definition of dma_map_single.

The last line here needs to be

rxbd-data = cpu_to_le32(addr);

which fixes the bug, and has no dependency on a 32 bit CPU.

Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread Joe Perches
On Fri, 2013-06-21 at 17:58 +0200, Arnd Bergmann wrote:
 On Friday 21 June 2013, Joe Perches wrote:
  On Fri, 2013-06-21 at 10:53 +, Alexey Brodkin wrote:
   On 06/21/2013 02:32 PM, Joe Perches wrote:
On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
instantiated in some legacy ARC (Synopsys) FPGA Boards such as
ARCAngel4/ML50x.
  []
+  rxbd-data = (unsigned char 
*)cpu_to_le32(rx_buff-skb-data);
   
32 bit only.  Should the Kconfig block have some arch_arc dependency
so it can't get compiled for 64 bit systems?
  []
   So for now I may easily add dependency on ARC if it makes acceptance of 
   driver easier.
  
  I don't think it's a big problem.
  
  Maybe add a Kconfig depends on !64BIT.
  
  Another thing to do would be to run it through sparse
  with make C=1
 
 No, I think the driver should just be made 64-bit clean. Not because
 anyone is going to need that of course, but to avoid having
 to add silly dependencies.
 
 The problem is that rxbd-data is the wrong type. It is declared
 as a void*, but it is not a kernel pointer at all, it is a DMA
 pointer. Look at this code in context:

So it should be declared dma_addr_t then,

 +   addr = dma_map_single(ndev-dev, (void *)rx_buff-skb-data,
 + buflen, DMA_FROM_DEVICE);
 +   if (dma_mapping_error(ndev-dev, addr)) {
 +   if (net_ratelimit())
 +   netdev_err(ndev, cannot dma map\n);
 +   dev_kfree_skb(rx_buff-skb);
 +   stats-rx_errors++;
 +   continue;
 +   }
 +   dma_unmap_addr_set(rx_buff, mapping, addr);
 +   dma_unmap_len_set(rx_buff, len, buflen);
 +
 +   rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);
 
 the 'addr' returned by dma_map_single is what the device really
 needs, although it is the same as rx_buff-skb-data with the
 trivial definition of dma_map_single.
 
 The last line here needs to be
 rxbd-data = cpu_to_le32(addr);
 
 which fixes the bug, and has no dependency on a 32 bit CPU.

It still has a dependency on dma_addr_t size being 32 bit



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


RE: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread David Laight
 So it should be declared dma_addr_t then,
 
  +   addr = dma_map_single(ndev-dev, (void 
  *)rx_buff-skb-data,
  + buflen, DMA_FROM_DEVICE);
  +   if (dma_mapping_error(ndev-dev, addr)) {
  +   if (net_ratelimit())
  +   netdev_err(ndev, cannot dma map\n);
  +   dev_kfree_skb(rx_buff-skb);
  +   stats-rx_errors++;
  +   continue;
  +   }
  +   dma_unmap_addr_set(rx_buff, mapping, addr);
  +   dma_unmap_len_set(rx_buff, len, buflen);
  +
  +   rxbd-data = (unsigned char 
  *)cpu_to_le32(rx_buff-skb-data);
 
  the 'addr' returned by dma_map_single is what the device really
  needs, although it is the same as rx_buff-skb-data with the
  trivial definition of dma_map_single.
 
  The last line here needs to be
  rxbd-data = cpu_to_le32(addr);
 
  which fixes the bug, and has no dependency on a 32 bit CPU.
 
 It still has a dependency on dma_addr_t size being 32 bit

No - just a dependency on memory being mapped to a 32bit
physical address accessible from the device.
Many 64 bit systems have devices that can only access 32bit
address space, either the memory has to be allocated from the
correct arena, of DMA bounce buffers are used.

David



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


Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver

2013-06-21 Thread David Miller
From: Joe Perches j...@perches.com
Date: Fri, 21 Jun 2013 07:43:33 -0700

 Maybe add a Kconfig depends on !64BIT.

Please, no.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/