Re: [PATCH v5.1 1/2] staging: ion: Add generic ion-physmem driver

[Andrew]

Thanks for the detailed review

Laura Abbott писал 30.06.2015 20:56:

On 06/30/2015 08:34 AM, Andrew Andrianov wrote:

if (!idev)

+   return -ENOMEM;
+   }


Yeah this is a bit messy as your comments note. Since there can only be 
one Ion
device in the system, it seems like it would make more sense to have a 
top level
DT node and then have the heaps be subnodes to avoid this 'guess when 
to create

the device' bit.



I'm afraid this is not a very good idea, if the heaps represent some 
_physical_

address ranges, e.g. a SRAM memory (read below for our weird use case).
In this case, the way I understand DT philosophy, it should be a subnode 
of the
respective axi/apb/whatever node where it's connected. Correct me if I'm 
wrong.



+
+   ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL);
+   if (!ipdev)
+   return -ENOMEM;
+
+   platform_set_drvdata(pdev, ipdev);
+
+   /* Read out name first for the sake of sane error-reporting */
+   ret =  of_property_read_string(pdev->dev.of_node, "ion-heap-name",
+  &ion_heap_name);
+   if (ret != 0)
+   goto errfreeipdev;
+
+   ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-id",
+   &ion_heap_id);
+   if (ret != 0)
+   goto errfreeipdev;
+
+   /* Check id to be sane first */
+   if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) {
+   dev_err(&pdev->dev, "bad heap id specified: %d\n",
+   ion_heap_id);
+   goto errfreeipdev;
+   }
+
+   if ((1 << ion_heap_id) & claimed_heap_ids) {
+   dev_err(&pdev->dev, "heap id %d is already claimed\n",
+   ion_heap_id);
+   goto errfreeipdev;
+   }


I thought the core Ion framework was already checking this but
apparently not. This is so fundamental it should really be moved into
the core framework and not at the client level.


I tried to mess as little as possible (e.g. not mess at all) with the 
guts of
ION, so ended up with an extra check. If you want, I can hack into the 
ion itself,

and send the patch for the next respin.




+
+   ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-type",
+   &ion_heap_type);
+   if (ret != 0)
+   goto errfreeipdev;
+
+   ret =  of_property_read_u32(pdev->dev.of_node, "ion-heap-align",
+   &ion_heap_align);
+   if (ret != 0)
+   goto errfreeipdev;
+
+   res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+   /* Not always needed, throw no error if missing */
+   if (res) {
+   /* Fill in some defaults */
+   addr = res->start;
+   size = resource_size(res);
+   }
+
+   switch (ion_heap_type) {
+   case ION_HEAP_TYPE_DMA:
+   if (res) {
+   ret = dma_declare_coherent_memory(&pdev->dev,
+ res->start,
+ res->start,
+ resource_size(res),
+ DMA_MEMORY_MAP |
+ DMA_MEMORY_EXCLUSIVE);
+   if (ret == 0) {
+   ret = -ENODEV;
+   goto errfreeipdev;
+   }
+   }


The name is ION_HEAP_TYPE_DMA but the real point of the heap was to
be able to use CMA memory. Calling dma_declare_coherent_memory defeats
the point since we won't use CMA memory. The coherent memory pool is 
closer

to a carveout anyway so I'd argue if you want the effects of a coherent
memory pool you should be using carveout memory anyway.


In our weird use case we use ION to share buffers between NeuroMatrix 
DSP
cores, video decoder, video output device and a userspace application 
that
orchestrates the whole process. The system has several dedicated SRAM 
banks,
that should represented as dedicated ION heaps. Those are differently 
wired in
the system (e.g. ARM core can't even execute code from some of them) and 
to
achieve maximum performance certain buffers should be only allocated 
from

certain banks for the thing to work fast.
(Yes, it's just as scary as it sounds)

In other words - we need several coherent pools, and 
dma_declare_coherent

looked like the only existing way to tell ION:
"hey, we want a heap out of this very physical region!"

In reality that's mostly our only use case, all the rest heap types look 
like mostly
useful for testing ra

Re: [PATCH v5.1 2/2] staging: ion: Add ion-physmem documentation

[Andrew]

Laura Abbott писал 30.06.2015 20:54:

(adding devicetree mailing list since I didn't see it cc-ed)

Please also remember to cc the staging list since Ion is
still a staging framework.

On 06/30/2015 08:34 AM, Andrew Andrianov wrote:

Signed-off-by: Andrew Andrianov 
---
  Documentation/devicetree/bindings/ion,physmem.txt | 98 
+++


The proper place is bindings/staging/ since Ion is still a staging 
driver


Got it, will fix and send for the next respin




  1 file changed, 98 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/ion,physmem.txt

diff --git a/Documentation/devicetree/bindings/ion,physmem.txt 
b/Documentation/devicetree/bindings/ion,physmem.txt

new file mode 100644
index 000..e8c64dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/ion,physmem.txt
@@ -0,0 +1,98 @@
+ION PhysMem Driver
+#include 
+
+
+ION PhysMem is a generic driver for ION Memory Manager that allows 
you to
+define ION Memory Manager heaps using device tree. This is mostly 
useful if
+your SoC has several 'special' regions (e.g. SRAM, dedicated memory 
banks,
+etc) that are present in the physical memory map and you want to add 
them to

+ION as heaps of memory.
+


A good start of a description. This could use a bit more detail about 
what the
Ion memory framework actually does (i.e. tied really strongly to 
Android)


Ironically we use ION without android. We even started of a liblinuxion
for use in traditional linux userspace (should be up at RC Module's 
github pretty soon)

I'll add a bit more words here, that's not a problem.



You are also missing a generic description of what all goes into the 
binding.

Based on what you have below you would get

(name) : ion@(base-address) {
compatible = "ion,physmem";
reg = <(baseaddr) (size)>;
reg-names = "memory";
ion-heap-id = <(int-value)>;
ion-heap-type = <(int-value)>;
ion-heap-align = <(int-value)>;
ion-heap-name = "(string value");
};

and then you need to describe what each of those properties actually 
do.
Having all the examples is still really useful, especially for heaps 
such as the

system heaps which are independent of any memory map.


Got it, will fix.


+
+Examples:
+
+1. 256KiB On-chip SRAM used as ION DMA heap. reg range is treated as 
a physical

+   address range
+
+   ion_im0: ion@0x0010 {
+compatible = "ion,physmem";
+reg = <0x0010 0x4>;
+reg-names = "memory";
+ion-heap-id   = <2>;
+ion-heap-type = ;
+ion-heap-align = <0x10>;
+ion-heap-name = "IM0";
+   };
+
+2. The same, but using system DMA memory.
+
+   ion_dma: ion@0xdeadbeef {
+compatible = "ion,physmem";
+ion-heap-id   = <2>;
+ion-heap-type = ;
+ion-heap-align = <0x10>;
+ion-heap-name = "SYSDMA";
+   };


CMA now has bindings upstream. I'd rather see Ion be a CMA client 
instead

of creating any other bindings.


Unfortunately, this breaks the most useful case for us, when ion uses
several dedicated physical memory areas. Maybe wrap CMA and use it as
another ion-heap-type then?




+
+3. Carveout heap, 1MiB size, ion-physmem will alloc pages for it 
using

+   alloc_pages_exact(). reg range is used for specifying size only.
+
+   ion_crv: ion@deadbeef {
+compatible = "ion,physmem";
+reg = <0x 0x10>;
+reg-names = "memory";
+ion-heap-id   = <3>;
+ion-heap-type = ;
+ion-heap-align = <0x10>;
+ion-heap-name = "carveout";
+   };
+


My understanding of DT binding rules was that for foo@X, your reg must
equal X. Maintainers can correct me if I'm wrong or if that restriction
is relaxed these days.


In case reg doesn't represent a physical memory region, but only size 
here
(for convenient resource_size calls) we may end with several ion@0 this 
way.

Is it really required to be so?




+4. Chunk heap. 1MiB size, ion-physmem will alloc pages for it using
+   alloc_pages_exact(). reg range is used for specifying size only.
+
+   ion_chunk: ion@0xdeadbeef {
+compatible = "ion,physmem";
+ion-heap-id   = <2>;
+ion-heap-type = ;
+ion-heap-align = <0x10>;
+ion-heap-name = "chunky";
+   };
+
+
+5. vmalloc();
+
+   ion_chunk: ion@0xdeadbeef {
+compatible = "ion,physmem";
+ion-heap-id   = <2>;
+ion-heap-type = ;
+ion-heap-align = <0x10>;
+ion-heap-name = "sys";
+   };
+
+6. kmalloc();
+
+   ion_ch

Re: [RFC][PATCH 2/2] staging: ion: Add files for parsing the devicetree (WIP)

[Andrew]

Thanks a lot for starting again with dt bindings discussion.
I got carried away by work and never had a chance for a respin of my 
ion-physmem.


I'll try to test these on actual hardware next week and provide more 
detailed

feedback. For now just a small pick:


-
+obj-$(CONFIG_ION_DUMMY)   += ion_dummy_driver.o
+obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o
+obj-$(CONFIG_ION_TEGRA)   += tegra/
+obj-$(CONFIG_ION_OF) +=ion_of.o ion_physmem.o


ion_physmem.o looks like the one that shouldn't be here, right?

P.S. Anyone interested in a non-android port of libion with
a few helper utils? I had to call it liblinuxion, since libion is 
something

already present in repositories.

--
Regards,
Andrew
RC Module
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion

[Andrew]

On 2015-10-07 02:01, Laura Abbott wrote:

On 10/6/15 3:35 PM, Rob Herring wrote:
On Tue, Oct 6, 2015 at 3:47 PM, Laura Abbott 
 wrote:

From: Laura Abbott 


This adds a base set of devicetree bindings for the Ion memory
manager. This supports setting up the generic set of heaps and
their properties.

Signed-off-by: Laura Abbott 
Signed-off-by: Andrew Andrianov 
---
  drivers/staging/android/ion/devicetree.txt | 53 
++


I have no issue with this going in here, but I do have lots of issues
with this binding.


  1 file changed, 53 insertions(+)
  create mode 100644 drivers/staging/android/ion/devicetree.txt

diff --git a/drivers/staging/android/ion/devicetree.txt 
b/drivers/staging/android/ion/devicetree.txt

new file mode 100644
index 000..4a0c941
--- /dev/null
+++ b/drivers/staging/android/ion/devicetree.txt
@@ -0,0 +1,53 @@
+Ion Memory Manager
+
+Ion is a memory manager that allows for sharing of buffers via 
dma-buf.
+Ion allows for different types of allocation via an abstraction 
called

+a 'heap'. A heap represents a specific type of memory. Each heap has
+a different type. There can be multiple instances of the same heap
+type.
+
+Required properties for Ion
+
+- compatible: "linux,ion"
+
+All child nodes of a linux,ion node are interpreted as heaps
+
+required properties for heaps
+
+- linux,ion-heap-id: The Ion heap id used for allocation selection
+- linux,ion-heap-type: Ion heap type defined in ion.h
+- linux,ion-heap-name: Human readble name of the heap
+
+
+Optional properties
+- memory-region: A phandle to a memory region. Required for DMA heap 
type

+(see reserved-memory.txt for details on the reservation)
+- linux,ion-heap-align: Alignment for the heap.
+
+Example:
+
+   ion {
+   compatbile = "linux,ion";
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   ion-system-heap {
+   linux,ion-heap-id = <0>;
+   linux,ion-heap-type = ;
+   linux,ion-heap-name = "system";


How does this vary across platforms? Is all of this being pushed down
to DT, because there is no coordination of this at the kernel ABI
level across platforms. In other words, why can't heap 0 be hardcoded
as system heap in the driver. It seems to me any 1 of these 3
properties could be used to derive the other 2.



Right now there is no guarantee heap IDs will be the same across
platforms. The heap IDs are currently part of the userspace ABI
as well since userspace clients must pass in a mask of the heap
IDs to allocate from. If we assume all existing clients could change,
heaps such as the system heap could be mandated to have the same
heap ID but we'd still run into problems if you have multiple
heaps of the same type (e.g. multiple carveouts)


I don't really like the idea of enforcing any IDs here. As of now
heap ids are generally something VERY platform-specific
(or even product-specific). Personally I'd prefer something like this
for userspace apps:

int id1 = ion_get_heap_id("camera");
if (id1 < 0) {
  fprintf(stderr, "Invalid heap id");
  exit(1);
}

int id2 = ion_get_heap_id("backup-heap");
if (id2 < 0) {
  fprintf(stderr, "Invalid heap id");
  exit(1);
}

...

int ret = ion_alloc(fd, 0x100, 0x4,
  (id1 | id2),
  0, &handle);


What concerns kernel stuff, things are simpler - we may just pass the 
heap to use

by a reference in devicetree node for that driver. Something like that:

...
   ion-decoder-region : region_ddr {
   linux,ion-heap-id = <1>;
   linux,ion-heap-type = ;
   linux,ion-heap-name = "decoder_mem"
   memory-region = <&camera_region>;
};
...
video_decoder@8018 {
compatible = "acme,h266dec";
reg = <0x8018 0x2>,
reg-names = "registers";
interrupts = <12>;
interrupt-parent = <&vic1>;
ion-heaps-for-buffers = <&ion-decoder-region>
};





An alternative might be to have each heap just be a compatible string
and pull everything (id, type etc.) into C files for setup. I debated
doing that but decided to try putting everything in DT for my first
pass.




+   };
+
+   ion-camera-region {
+   linux,ion-heap-id = <1>;
+   linux,ion-heap-type = ;
+   linux,ion-heap-name = "camera"
+   memory-region = <&camera_region>;


Couldn't the memory-region node with addition properties or some
standardizati

Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion

[Andrew]

On 2015-10-07 21:36, Rob Herring wrote:

On Wed, Oct 7, 2015 at 5:36 AM, Andrew  wrote:

On 2015-10-07 02:01, Laura Abbott wrote:


On 10/6/15 3:35 PM, Rob Herring wrote:


On Tue, Oct 6, 2015 at 3:47 PM, Laura Abbott 


wrote:


From: Laura Abbott 


This adds a base set of devicetree bindings for the Ion memory
manager. This supports setting up the generic set of heaps and
their properties.

Signed-off-by: Laura Abbott 
Signed-off-by: Andrew Andrianov 
---
  drivers/staging/android/ion/devicetree.txt | 53
++



I have no issue with this going in here, but I do have lots of 
issues

with this binding.


  1 file changed, 53 insertions(+)
  create mode 100644 drivers/staging/android/ion/devicetree.txt

diff --git a/drivers/staging/android/ion/devicetree.txt
b/drivers/staging/android/ion/devicetree.txt
new file mode 100644
index 000..4a0c941
--- /dev/null
+++ b/drivers/staging/android/ion/devicetree.txt
@@ -0,0 +1,53 @@
+Ion Memory Manager
+
+Ion is a memory manager that allows for sharing of buffers via 
dma-buf.
+Ion allows for different types of allocation via an abstraction 
called
+a 'heap'. A heap represents a specific type of memory. Each heap 
has

+a different type. There can be multiple instances of the same heap
+type.
+
+Required properties for Ion
+
+- compatible: "linux,ion"
+
+All child nodes of a linux,ion node are interpreted as heaps
+
+required properties for heaps
+
+- linux,ion-heap-id: The Ion heap id used for allocation selection
+- linux,ion-heap-type: Ion heap type defined in ion.h
+- linux,ion-heap-name: Human readble name of the heap
+
+
+Optional properties
+- memory-region: A phandle to a memory region. Required for DMA 
heap

type
+(see reserved-memory.txt for details on the reservation)
+- linux,ion-heap-align: Alignment for the heap.
+
+Example:
+
+   ion {
+   compatbile = "linux,ion";
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   ion-system-heap {
+   linux,ion-heap-id = <0>;
+   linux,ion-heap-type = 
;

+   linux,ion-heap-name = "system";



How does this vary across platforms? Is all of this being pushed 
down

to DT, because there is no coordination of this at the kernel ABI
level across platforms. In other words, why can't heap 0 be 
hardcoded

as system heap in the driver. It seems to me any 1 of these 3
properties could be used to derive the other 2.



Right now there is no guarantee heap IDs will be the same across
platforms. The heap IDs are currently part of the userspace ABI
as well since userspace clients must pass in a mask of the heap
IDs to allocate from. If we assume all existing clients could change,
heaps such as the system heap could be mandated to have the same
heap ID but we'd still run into problems if you have multiple
heaps of the same type (e.g. multiple carveouts)


Vendors largely ignore the kernel-userspace ABI and anything in
staging is not a ABI. So arguments about what the ABI is currently is
pointless IMO.

Pushing an inconsistent kernel ABI to DT is not the answer.


Totally agree here.





I don't really like the idea of enforcing any IDs here. As of now
heap ids are generally something VERY platform-specific
(or even product-specific). Personally I'd prefer something like this
for userspace apps:

int id1 = ion_get_heap_id("camera");
if (id1 < 0) {
  fprintf(stderr, "Invalid heap id");
  exit(1);
}

int id2 = ion_get_heap_id("backup-heap");
if (id2 < 0) {
  fprintf(stderr, "Invalid heap id");
  exit(1);
}


We've learned that creating number spaces like this are bad (irqs,
gpios, /dev nodes). We should move away from that. Why should
userspace care about IDs or what the IDs are? An ID is just encoding
certain implicit requirements. So are the strings here. Users should
express what capabilities, restrictions, etc. they have, and then the
kernel can find the best heap.


I'd argue about that point, since sometimes kernel might NOT know
all the hardware details behind some of the heaps or how they are going 
to
be used. Hence it can't decide a proper heap. And that's where things 
get ugly.


Real-world example: There are several on-chip SRAM banks that make up 
several

heaps. Say, IM0, IM1, IM2.

Technically - they are all DMA, and all work. But the hardware guys hand 
you

a handful of weird recommendations, like:
* Decoder will work faster if you use bank IM2 for internal buffers,
and prefer DDR bank A for decoded frames.
* DSP should stick with IM1, and please prefer DDR bank B for buffers

When several such devices are involved in one chain - things may get
even weirder. Having manual control over where allocations are made 
allows
us to keep all these voodoo magicks away from the kernel and (hopefully) 
keep

vendors from

Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion

[Andrew]

On 2015-10-12 21:39, Mitchel Humpherys wrote:
On Tue, Oct 06 2015 at 05:35:41 PM, Rob Herring  
wrote:
On Tue, Oct 6, 2015 at 3:47 PM, Laura Abbott 
 wrote:


[...]


+Example:
+
+   ion {
+   compatbile = "linux,ion";
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   ion-system-heap {
+   linux,ion-heap-id = <0>;
+   linux,ion-heap-type = ;
+   linux,ion-heap-name = "system";


How does this vary across platforms? Is all of this being pushed down
to DT, because there is no coordination of this at the kernel ABI
level across platforms. In other words, why can't heap 0 be hardcoded
as system heap in the driver. It seems to me any 1 of these 3
properties could be used to derive the other 2.


The heap-id<->heap-type mapping isn't necessarily 1:1.  As Laura
indicated elsewhere on this thread, a given heap might need to be
contiguous on one platform but not on another.  In that case you just
swap out the heap-type here and there's no need for userspace to 
change.


The heap-name, OTOH, could be derived from the heap-id, which is what 
we

hackishly do here [1] and here[2].


By the way, since we agreed that heap id and heap type mappings
are not 1:1 - we have a problem with the current API.

In userspace we currently have this:

int ion_alloc(int fd, size_t len, size_t align, unsigned int heap_mask,
  unsigned int flags, ion_user_handle_t *handle);

We do not specify here what TYPE of heap we want the allocation to come 
from.
This may lead to very unpleasant stuff when porting from one platfrom to 
another.


But, manual heap id control is VERY neat, since it allows to cover a lot 
of

platform-specific cases, where allocating memory from certain heaps can
yield performance boost.

In other words - maybe we should specify both suitable heap types and 
heap ids?



[1]
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.14/tree/drivers/staging/android/ion/msm/msm_ion.c?h=msm-3.14#n53
[2]
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.14/tree/drivers/staging/android/ion/msm/msm_ion.c?h=msm-3.14#n398


-Mitch


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

Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion

[andrew]

20 октября 2015 г., 19:34, "Mitchel Humpherys"  
написал:
> On Tue, Oct 13 2015 at 11:14:23 AM, Andrew  wrote:
> 
>> On 2015-10-12 21:39, Mitchel Humpherys wrote:
>>> On Tue, Oct 06 2015 at 05:35:41 PM, Rob Herring 
>>> wrote:
>>>> On Tue, Oct 6, 2015 at 3:47 PM, Laura Abbott 
>>>> wrote:
>>> 
>>> [...]
>>> 
>>>>> +Example:
>>>>> +
>>>>> + ion {
>>>>> + compatbile = "linux,ion";
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> +
>>>>> + ion-system-heap {
>>>>> + linux,ion-heap-id = <0>;
>>>>> + linux,ion-heap-type = ;
>>>>> + linux,ion-heap-name = "system";
>>>> 
>>>> How does this vary across platforms? Is all of this being pushed down
>>>> to DT, because there is no coordination of this at the kernel ABI
>>>> level across platforms. In other words, why can't heap 0 be hardcoded
>>>> as system heap in the driver. It seems to me any 1 of these 3
>>>> properties could be used to derive the other 2.
>>> 
>>> The heap-id<->heap-type mapping isn't necessarily 1:1. As Laura
>>> indicated elsewhere on this thread, a given heap might need to be
>>> contiguous on one platform but not on another. In that case you just
>>> swap out the heap-type here and there's no need for userspace to change.
>>> 
>>> The heap-name, OTOH, could be derived from the heap-id, which is what we
>>> hackishly do here [1] and here[2].
>> 
>> By the way, since we agreed that heap id and heap type mappings
>> are not 1:1 - we have a problem with the current API.
>> 
>> In userspace we currently have this:
>> 
>> int ion_alloc(int fd, size_t len, size_t align, unsigned int heap_mask,
>> unsigned int flags, ion_user_handle_t *handle);
>> 
>> We do not specify here what TYPE of heap we want the allocation to come
>> from.
>> This may lead to very unpleasant stuff when porting from one platfrom to
>> another.

Okay, I may be totally missing some point here then.

> What "unpleasant stuff" are you referring to, exactly? 

It's not really clear for me how (and at where - kernel or userspace) 
we should properly sort out cases when the next device the pipeline 
introduces some constraints on the buffer it can use. 

For instance: camera can save data into a non-contiguous buffer, but the 
image processing hardware (that may or may not be involved) expects the 
buffer to be contiguous.

> Abstracting the
> heap type away from userspace has actually made our lives easier since
> userspace doesn't need to know anything about the properties of the
> underlying platform. It just asks for a buffer from the "camera" heap,
> for example. On some platforms that's contiguous, on others it's not.
> 
> -Mitch
> 
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

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

Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/

[Andrew Lunn]

On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> This patch is to move DPAA2 PTP driver out of staging/
> since the dpaa2-eth had been moved out.
> 
> Signed-off-by: Yangbo Lu 
> ---
>  drivers/net/ethernet/freescale/Kconfig |9 +
>  drivers/net/ethernet/freescale/dpaa2/Kconfig   |   15 +++
>  drivers/net/ethernet/freescale/dpaa2/Makefile  |6 --
>  .../ethernet/freescale/dpaa2}/dprtc-cmd.h  |0
>  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c   |0
>  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h   |0
>  .../rtc => net/ethernet/freescale/dpaa2}/rtc.c |0
>  .../rtc => net/ethernet/freescale/dpaa2}/rtc.h |0
>  drivers/staging/fsl-dpaa2/Kconfig  |8 
>  drivers/staging/fsl-dpaa2/Makefile |1 -
>  drivers/staging/fsl-dpaa2/rtc/Makefile |7 ---
>  11 files changed, 20 insertions(+), 26 deletions(-)
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
>  rename drivers/{staging/fsl-dpaa2/rtc => 
> net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => 
> net/ethernet/freescale/dpaa2}/dprtc.c (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => 
> net/ethernet/freescale/dpaa2}/dprtc.h (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c 
> (100%)
>  rename drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.h 
> (100%)

Hi Yangbo

Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the
name, change it to ptp.[ch]. Also, some of the function names, and
structures, rtc_probe->ptp_probe, rtc_remove->ptp_remove,
rtc_match_id_table-> ptp_match_id_table, etc.

ptp_dpaa2_adjfreq() probably should return err, not 0.
ptp_dpaa2_gettime() again does not return the error.
If fact, it seems like all the main functions ignore errors.

kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
Can ptp_dpaa2_caps be made const?
dpaa2_phc_index does not appear to be used.
dev_set_drvdata(dev, NULL); is not needed.
Can rtc_drv be made const?
Is rtc.h used by anything other than rtc.c? It seems like it can be removed.

It seems like there is a lot of code in dprtc.c which is unused. rtc.c
does nothing with interrupts for example. Do you plan to make use of
this extra code? Or can it be removed leaving just what is needed?

struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct
seems very odd. And it is not the only example.

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

Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/

[Andrew Lunn]

> > struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct seems 
> > very
> > odd. And it is not the only example.
> 
> [Y.b. Lu] This should depended on MC firmware and APIs I think. Once the MC 
> improves this, the APIs could be updated to fix this.

That is going to be hard to do. Ideally the driver should work with
any firmware version. You don't really want to force the user to
upgrade the driver/kernel and the firmware at the same time. So you
cannot for example remove this pad. What you might be able to do in
newer versions is actually use the space. But you have to be sure the
current code is correctly ignoring it and setting it to zero.

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

Re: [v2, 1/5] net: dpaa2: move DPAA2 PTP driver out of staging/

[Andrew Lunn]

> +++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> @@ -0,0 +1,15 @@
> +config FSL_DPAA2_ETH
> + tristate "Freescale DPAA2 Ethernet"
> + depends on FSL_MC_BUS && FSL_MC_DPIO

Could you add in here COMPILE_TEST? 

> + depends on NETDEVICES && ETHERNET

With the move out of staging, i don't think these two are required.

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

Re: [PATCH v5 1/2] memory_hotplug: Free pages as higher order

[Andrew Morton]

On Thu, 11 Oct 2018 09:55:03 +0200 Michal Hocko  wrote:

> > > > > This is now not called anymore, although the xen/hv variants still do
> > > > > it. The function seems empty these days, maybe remove it as a followup
> > > > > cleanup?
> > > > >
> > > > > > -   __online_page_increment_counters(page);
> > > > > > -   __online_page_free(page);
> > > > > > +   __free_pages_core(page, order);
> > > > > > +   totalram_pages += (1UL << order);
> > > > > > +#ifdef CONFIG_HIGHMEM
> > > > > > +   if (PageHighMem(page))
> > > > > > +   totalhigh_pages += (1UL << order);
> > > > > > +#endif
> > > > >
> > > > > __online_page_increment_counters() would have used
> > > > > adjust_managed_page_count() which would do the changes under
> > > > > managed_page_count_lock. Are we safe without the lock? If yes, there
> > > > > should perhaps be a comment explaining why.
> > > > 
> > > > Looks unsafe without managed_page_count_lock.
> > > 
> > > Why does it matter actually? We cannot online/offline memory in
> > > parallel. This is not the case for the boot where we initialize memory
> > > in parallel on multiple nodes. So this seems to be safe currently unless
> > > I am missing something. A comment explaining that would be helpful
> > > though.
> > 
> > Other main callers of adjust_manage_page_count(),
> > 
> > static inline void free_reserved_page(struct page *page)
> > {
> > __free_reserved_page(page);
> > adjust_managed_page_count(page, 1);
> > }
> > 
> > static inline void mark_page_reserved(struct page *page)
> > {
> > SetPageReserved(page);
> > adjust_managed_page_count(page, -1);
> > }
> > 
> > Won't they race with memory hotplug?
> > 
> > Few more,
> > ./drivers/xen/balloon.c:519:adjust_managed_page_count(page, -1);
> > ./drivers/virtio/virtio_balloon.c:175:  adjust_managed_page_count(page, -1);
> > ./drivers/virtio/virtio_balloon.c:196:  adjust_managed_page_count(page, 1);
> > ./mm/hugetlb.c:2158:adjust_managed_page_count(page, 1 <<
> > h->order);
> 
> They can, and I have missed those.

So this patch needs more work, yes?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/2] memory_hotplug: Free pages as higher order

[Andrew Morton]

On Mon, 05 Nov 2018 15:12:27 +0530 Arun KS  wrote:

> On 2018-10-22 16:03, Arun KS wrote:
> > On 2018-10-19 13:37, Michal Hocko wrote:
> >> On Thu 18-10-18 19:18:25, Andrew Morton wrote:
> >> [...]
> >>> So this patch needs more work, yes?
> >> 
> >> Yes, I've talked to Arun (he is offline until next week) offlist and 
> >> he
> >> will play with this some more.
> > 
> > Converted totalhigh_pages, totalram_pages and zone->managed_page to
> > atomic and tested hot add. Latency is not effected with this change.
> > Will send out a separate patch on top of this one.
> Hello Andrew/Michal,
> 
> Will this be going in subsequent -rcs?

I thought were awaiting a new version?  "Will send out a separate patch
on top of this one"?

I do think a resend would be useful, please.  Ensure the changelog is
updated to capture the above info and any other worthy issues which
arose during review.

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

Re: [PATCH V2] binder: ipc namespace support for android binder

[Andrew Morton]

On Mon, 29 Oct 2018 06:18:11 + chouryzhou(周威)  
wrote:

>   We are working for running android in container, but we found that binder is
> not isolated by ipc namespace. Since binder is a form of IPC and therefore 
> should
> be tied to ipc namespace. With this patch, we can run more than one android
> container on one host.
>   This patch move "binder_procs" and "binder_context" into ipc_namespace,
> driver will find the context from it when opening. Althought statistics in 
> debugfs
> remain global.
> 
> ...
>
> --- a/ipc/namespace.c
> +++ b/ipc/namespace.c
> @@ -56,6 +56,9 @@ static struct ipc_namespace *create_ipc_ns(struct 
> user_namespace *user_ns,
> ns->ucounts = ucounts;
>  
> err = mq_init_ns(ns);
> +   if (err)
> +   goto fail_put;
> +   err = binder_init_ns(ns);
> if (err)
> goto fail_put;
>  

Don't we need an mq_put_mnt() if binder_init_ns() fails?

free_ipc_ns() seems to have forgotten about that too.  In which case it
must be madly leaking mounts so probably I'm wrong.  Confused.

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

Re: [PATCH V2] binder: ipc namespace support for android binder

[Andrew Morton]

On Wed, 7 Nov 2018 01:48:12 + chouryzhou(周威)  wrote:

> > > --- a/ipc/namespace.c
> > > +++ b/ipc/namespace.c
> > > @@ -56,6 +56,9 @@ static struct ipc_namespace *create_ipc_ns(struct
> > user_namespace *user_ns,
> > > ns->ucounts = ucounts;
> > >
> > > err = mq_init_ns(ns);
> > > +   if (err)
> > > +   goto fail_put;
> > > +   err = binder_init_ns(ns);
> > > if (err)
> > > goto fail_put;
> > >
> > 
> > Don't we need an mq_put_mnt() if binder_init_ns() fails?
> > 
> > free_ipc_ns() seems to have forgotten about that too.  In which case it
> > must be madly leaking mounts so probably I'm wrong.  Confused.
> > 
> 
> mq_init_ns will do clean job if it failed, and as do binder_init_ns. 

My point is that if mq_init_ns() succeeds and binder_init_ns() fails,
we don't undo the effects of mq_init_ns()?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V4] binder: ipc namespace support for android binder

[Andrew Morton]

On Mon, 12 Nov 2018 09:37:51 + chouryzhou(周威)  
wrote:

> Currently android's binder is not isolated by ipc namespace. Since binder 
> is a form of IPC and therefore should be tied to ipc namespace. With this 
> patch, we can run multiple instances of  android container on one host.
> 
> This patch move "binder_procs" and "binder_context" into ipc_namespace,
> driver will find the context from it when opening. For debugfs, binder_proc
> is namespace-aware, but not for binder dead nodes, binder_stats and 
> binder_transaction_log_entry (we added ipc inum to trace it).
> 
> ...
>
>  drivers/android/binder.c  | 133 
> --
>  include/linux/ipc_namespace.h |  15 +
>  ipc/namespace.c   |  10 +++-
>  3 files changed, 125 insertions(+), 33 deletions(-)

Well, it's mainly an android patch so I suggest this be taken via the
android tree.

Acked-by: Andrew Morton 

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

RE; READ AND REPLY

[ANDREW TOCHAILE]

Good Day,


I have a business proposal for you and it's 100% legal.




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

RE; READ AND REPLY

[ANDREW TOCHAILE]

Good Day,


I have a business proposal for you and it's 100% legal.




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

Re: [PATCH v2 net] staging: octeon: repair "fixed-link" support

[Andrew Lunn]

> + if (priv->of_node && 
> of_phy_is_fixed_link(priv->of_node)) {
> + if (of_phy_register_fixed_link(priv->of_node)) {
> + netdev_err(dev, "Failed to register 
> fixed link for interface %d, port %d\n",
> +interface, priv->port);
> + dev->netdev_ops = NULL;
> + }
> + }
> +
>   if (!dev->netdev_ops) {
>   free_netdev(dev);

Setting dev->netdev_ops to NULL to indicate an error is pretty
odd. But this is staging. How about cleaning this
up. of_phy_register_fixed_link() returns an -errno which you should
return.

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

Re: [PATCH v2 net] staging: octeon: repair "fixed-link" support

[Andrew Lunn]

> --- a/drivers/staging/octeon/ethernet.c
> +++ b/drivers/staging/octeon/ethernet.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -892,6 +893,14 @@ static int cvm_oct_probe(struct platform_device *pdev)
>   break;
>   }
>  
> + if (priv->of_node && 
> of_phy_is_fixed_link(priv->of_node)) {
> + if (of_phy_register_fixed_link(priv->of_node)) {
> + netdev_err(dev, "Failed to register 
> fixed link for interface %d, port %d\n",
> +interface, priv->port);
> + dev->netdev_ops = NULL;
> + }
> + }
> +
>   if (!dev->netdev_ops) {
>   free_netdev(dev);
>   } else if (register_netdev(dev) < 0) {
> -- 
> 2.10.2

I would also expect a call to of_phy_deregister_fixed_link() somewhere
in the driver.

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

Re: [PATCH v2 net] staging: octeon: Drop on uncorrectable alignment or FCS error

[Andrew Lunn]

> diff --git a/drivers/staging/octeon/ethernet-rx.c 
> b/drivers/staging/octeon/ethernet-rx.c
> index 2c16230..9ebd665 100644
> --- a/drivers/staging/octeon/ethernet-rx.c
> +++ b/drivers/staging/octeon/ethernet-rx.c
> @@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe 
> *work)
>   else
>   port = work->word1.cn38xx.ipprt;
>  
> - if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
> + if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))

It would be nice to replace all these err_code magic numbers with #defines.

You should also replace 64 with ETH_ZLEN + ETH_FCS_LEN. I also wonder
if <= should be just < ?

>   /*
>* Ignore length errors on min size packets. Some
>* equipment incorrectly pads packets to 64+4FCS
>* instead of 60+4FCS.  Note these packets still get
>* counted as frame errors.
>*/
> - } else if (work->word2.snoip.err_code == 5 ||
> -work->word2.snoip.err_code == 7) {
> + return 0;
> +
> + if (work->word2.snoip.err_code == 5 ||
> + work->word2.snoip.err_code == 7) {
>   /*
>* We received a packet with either an alignment error
>* or a FCS error. This may be signalling that we are
> @@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct 
> cvmx_wqe *work)
>   /* Port received 0xd5 preamble */
>   work->packet_ptr.s.addr += i + 1;
>   work->word1.len -= i + 5;
> - } else if ((*ptr & 0xf) == 0xd) {
> + return 0;
> + }
> +
> + if ((*ptr & 0xf) == 0xd) {

The comments are not so clear what is going on here. Can this
incorrectly match a destination MAC address of xD:XX:XX:XX:XX:XX.

>       /* Port received 0xd preamble */
>   work->packet_ptr.s.addr += i;
>   work->word1.len -= i + 4;

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

[PATCH] Android: binder: added a missing blank line after declaration

[Andrew Bridges]

Fixed a coding style issue.

Signed-off-by: Andrew Bridges 
---
 drivers/android/binder.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index b5117576792b..3241d233a12d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3614,6 +3614,7 @@ static int binder_thread_write(struct binder_proc *proc,
ret = -1;
if (increment && !target) {
struct binder_node *ctx_mgr_node;
+
mutex_lock(&context->context_mgr_node_lock);
ctx_mgr_node = context->binder_context_mgr_node;
if (ctx_mgr_node) {
-- 
2.25.1

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

Re: [PATCH] staging: mt7621-dts: remove obsolete switch node

[Andrew Lunn]

On Fri, Jan 08, 2021 at 10:51:55AM +0800, DENG Qingfang wrote:
> This was for OpenWrt's swconfig driver, which never made it upstream,
> and was also superseded by MT7530 DSA driver.

What about
Documentation/devicetree/bindings/net/mediatek,mt7620-gsw.txt ?
Should that also be removed?

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

Example code to access IRQ 5 on ISA card (using uio_pdrv_genirq ?)

[Andrew Brooks]

Hi

I would like to make use of an ISA card which generates IRQ 5
preferably using user-space code only, if possible.

Does anybody know of any example code using uio_pdrv_genirq
which could be adapted for this purpose?

Or, does anyone have a simple example kernel module?

Thanks!

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

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

On Sun, Dec 16, 2018 at 08:57:40AM +0100, Sergio Paracuellos wrote:
> +static int ksz_i2c_read_reg(struct i2c_client *client, u32 reg, u8 *val,
> + unsigned int len)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + struct i2c_msg msg[2];
> + u8 txd[2];

Hi Sergio

I'm not sure that having the TX buffer on the stack is safe. If the
i2c bus master is using DMA, you then DMA from the stack, which some
architectures memory models do no allow. You have to use memory which
comes from an alloc function.

> + int ret;
> +
> + txd[0] = (u8)(reg >> 8);
> + txd[1] = (u8)reg;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 2;
> + msg[0].buf = txd;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = len;
> + msg[1].buf = val;

You potentially have the same issue with val.

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

Re: [PATCH 2/2] dt-bindings: net: dsa: ksz9477: add sample of switch bindings managed in i2c mode

[Andrew Lunn]

On Sun, Dec 16, 2018 at 08:57:41AM +0100, Sergio Paracuellos wrote:
> Add device-tree binding example of the ksz9477 switch managed in i2c mode.
> 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Sergio Paracuellos 
> ---
>  .../devicetree/bindings/net/dsa/ksz.txt   | 50 +++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt 
> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> index 0f407fb371ce..9e715358cebb 100644
> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> @@ -74,3 +74,53 @@ Ethernet switch connected via SPI to the host, CPU port 
> wired to eth0:
>   };
>   };
>   };
> +
> +Ethernet switch connected via I2C to the host, CPU port wired to eth0:
> +
> + eth0: ethernet@10001000 {
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
> +
> + i2c0: i2c@f8008000 {
> + ksz9897: ksz9897@0 {

Hi Sergio

You should use a generic name here. Plus the @X needs to be the same as the reg 
value.
So switch: ksz9897@5f.

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

Re: [PATCH 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

> I'll change these two to to get memory from kernel allocators instead
> of using the stack. Thanks for let me know this.

There appear to be other cases as well. Please review the whole
driver.

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

Re: [PATCH v2 1/2] net: dsa: ksz9477: add I2C managed mode support

[Andrew Lunn]

> +
> + return ret;
> +}
> +
> +static int ksz_i2c_read32(struct ksz_device *dev, u32 reg, u32 *val)
> +{
> + int ret = ksz_i2c_read(dev, reg, (u8 *)val, 4);
> +
> + if (!ret)
> + *val = be32_to_cpu(*val);
> +
> + return ret;
> +}
> +
> +/**
> + * ksz_i2c_write_reg - issue write register command
> + * @client: i2c client structure
> + * @reg: The register address.
> + * @val: value to write
> + * @len: The length of data
> + *
> + * This is the low level write call that issues the necessary i2c message(s)
> + * to write data to the register specified in @reg.
> + */
> +static int ksz_i2c_write_reg(struct i2c_client *client, u32 reg, u8 *val,
> +  unsigned int len)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + struct i2c_msg msg;
> + int ret;
> +
> + msg.addr = client->addr;
> + msg.flags = 0;
> + msg.len = 2 + len;
> + msg.buf = val;
> +
> + ret = i2c_transfer(adapter, &msg, 1);
> + return (ret != 1) ? -EREMOTEIO : 0;
> +}
> +
> +static int ksz_i2c_write(struct ksz_device *dev, u32 reg, u8 *val,
> +  unsigned int len)
> +{
> + struct i2c_client *client = dev->priv;
> + unsigned int cnt = len;
> + int i;
> +
> + len = (len > I2C_TX_BUF_LEN) ? I2C_TX_BUF_LEN : len;
> + dev->txbuf[0] = (u8)(reg >> 8);
> + dev->txbuf[1] = (u8)reg;
> + i = 2;
> +
> + do {
> + dev->txbuf[i++] = (u8)(*val >> (8 * (cnt - 1)));
> + cnt--;
> + } while (cnt);
> +
> + return ksz_i2c_write_reg(client, reg, dev->txbuf, len);
> +}
> +
> +static int ksz_i2c_write8(struct ksz_device *dev, u32 reg, u8 value)
> +{
> + return ksz_i2c_write(dev, reg, &value, 1);
> +}
> +
> +static int ksz_i2c_write16(struct ksz_device *dev, u32 reg, u16 value)
> +{
> + value = cpu_to_be16(value);
> + return ksz_i2c_write(dev, reg, (u8 *)&value, 2);
> +}
> +
> +static int ksz_i2c_write24(struct ksz_device *dev, u32 reg, u32 value)
> +{
> + /* make it to big endian 24bit from MSB */
> + value <<= 8;
> + value = cpu_to_be32(value);
> +
> + return ksz_i2c_write(dev, reg, (u8 *)&value, 3);
> +}
> +
> +static int ksz_i2c_write32(struct ksz_device *dev, u32 reg, u32 value)
> +{
> + value = cpu_to_be32(value);
> + return ksz_i2c_write(dev, reg, (u8 *)&value, 4);
> +}
> +
> +static int ksz_i2c_get(struct ksz_device *dev, u32 reg, void *data, size_t 
> len)
> +{
> + return ksz_i2c_read(dev, reg, data, len);
> +}
> +
> +static int ksz_i2c_set(struct ksz_device *dev, u32 reg, void *data, size_t 
> len)
> +{
> + return ksz_i2c_write(dev, reg, data, len);
> +}
> +
> +static const struct ksz_io_ops ksz_i2c_ops = {
> + .read8 = ksz_i2c_read8,
> + .read16 = ksz_i2c_read16,
> + .read24 = ksz_i2c_read24,
> + .read32 = ksz_i2c_read32,
> + .write8 = ksz_i2c_write8,
> + .write16 = ksz_i2c_write16,
> + .write24 = ksz_i2c_write24,
> + .write32 = ksz_i2c_write32,
> +     .get = ksz_i2c_get,
> + .set = ksz_i2c_set,
> +};
> +
> +static int ksz_i2c_probe(struct i2c_client *client,
> +  const struct i2c_device_id *id)
> +{
> + struct ksz_device *dev;
> + int ret;
> +
> + dev = ksz_switch_alloc(&client->dev, &ksz_i2c_ops, client);
> + if (!dev)
> + return -ENOMEM;
> +
> + if (client->dev.platform_data)
> + dev->pdata = client->dev.platform_data;
> +
> + dev->txbuf = devm_kzalloc(dev->dev, 2 + I2C_TX_BUF_LEN, GFP_KERNEL);
> + if (!dev->txbuf)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, dev);
> +
> + ret = ksz9477_switch_register(dev);
> + if (ret) {
> + dev_err(&client->dev, "registering switch (ret: %d)\n", ret);
> + return ret;
> + }

Dan's comments appear to apply to this version as well.

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

Re: [PATCH] dt-bindings: net: dsa: ksz9477: fix indentation for switch spi bindings

[Andrew Lunn]

On Tue, Dec 25, 2018 at 12:45:11PM +0100, Sergio Paracuellos wrote:
> Hi David,
> 
> On Mon, Dec 24, 2018 at 11:15 PM David Miller  wrote:
> >
> > From: Sergio Paracuellos 
> > Date: Sat, 22 Dec 2018 08:39:09 +0100
> >
> > > Switch bindings for spi managed mode are using spaces instead of tabs.
> > > Fix them to get a file with a proper kernel indentation style.
> > >
> > > Signed-off-by: Sergio Paracuellos 
> >
> > This doesn't apply to any of my trees so I'm going to assume this was
> > targetting the devicetree GIT tree or something like that.
> 
> Actually, this was rebased onto linux-next. Which tree do you want me
> to rebase onto?

Hi Sergio

It should be based on net-next. However, that is closed now, so please
wait until it reopens in about two weeks time.

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

Re: [PATCH v2] staging: mt7621-eth/ethtool.c: Correction of SPDX license identifier

[Andrew Lunn]

On Wed, Jan 30, 2019 at 02:48:27PM +, Carlos Henrique Lima Melara wrote:
>   This patch fix the checkpatch.p1 warning:
> 
>   WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
>   +/*
> 
>   It includes the SPDX expression for GPL-2.0 and corrects the comment 
> format to suit the kernel's coding style.
> 
> Signed-off-by: Carlos (Charles) Henrique Lima Melara 
> 
> ---
>  drivers/staging/mt7621-eth/ethtool.c | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/mt7621-eth/ethtool.c 
> b/drivers/staging/mt7621-eth/ethtool.c
> index 40a7d47be913..49d417de64c8 100644
> --- a/drivers/staging/mt7621-eth/ethtool.c
> +++ b/drivers/staging/mt7621-eth/ethtool.c
> @@ -1,15 +1,17 @@
> -/*   This program is free software; you can redistribute it and/or modify
> - *   it under the terms of the GNU General Public License as published by
> - *   the Free Software Foundation; version 2 of the License
> +// SPDX-License-Identifier: GPL-2.0

Hi Carlos

drivers/staging/mt7621-eth$ grep LICENSE *
gsw_mt7621.c:MODULE_LICENSE("GPL");
mtk_eth_soc.c:MODULE_LICENSE("GPL");

And include/linux/module.h 
says:

/*
 * The following license idents are currently accepted as indicating free
 * software modules
 *
 *  "GPL"   [GNU Public License v2 or later]
 *  "GPL v2"    [GNU Public License v2]

So the SPDX string probably does not match the MODULE_LICENSE.

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

Re: [PATCH v2] staging: mt7621-eth/ethtool.c: Correction of SPDX license identifier

[Andrew Lunn]

> See the patch about all of this from Thomas on lkml yesterday for
> why this is the case.

Hi Greg

Thanks for the info. I had not seen this, and i guess other have not
as well. So here is a link to the patch.

https://lkml.org/lkml/2019/1/28/1975

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

Re: [PATCH v2 0/8] mm/kdump: allow to exclude pages that are logically offline

[Andrew Morton]

On Wed, 27 Feb 2019 13:32:14 +0800 Dave Young  wrote:

> This series have been in -next for some days, could we get this in
> mainline? 

It's been in -next for two months?

> Andrew, do you have plan about them, maybe next release?

They're all reviewed except for "xen/balloon: mark inflated pages
PG_offline". 
(https://ozlabs.org/~akpm/mmotm/broken-out/xen-balloon-mark-inflated-pages-pg_offline.patch).
Yes, I plan on sending these to Linus during the merge window for 5.1

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

Re: [PATCH] staging: octeon-ethernet: fix incorrect PHY mode

[Andrew Lunn]

> -static void cvm_set_rgmii_delay(struct device_node *np, int iface, int port)
> +static void cvm_set_rgmii_delay(struct octeon_ethernet *priv, int iface,
> + int port)
>  {
> + struct device_node *np = priv->of_node;
>   u32 delay_value;
> + bool rx_delay;
> + bool tx_delay;
>  
> - if (!of_property_read_u32(np, "rx-delay", &delay_value))
> + /* By default, both RX/TX delay is enabled in
> +  * __cvmx_helper_rgmii_enable().
> +  */
> + rx_delay = true;
> + tx_delay = true;
> +
> + if (!of_property_read_u32(np, "rx-delay", &delay_value)) {
>   cvmx_write_csr(CVMX_ASXX_RX_CLK_SETX(port, iface), delay_value);
> - if (!of_property_read_u32(np, "tx-delay", &delay_value))
> + rx_delay = delay_value > 0;
> + }
> + if (!of_property_read_u32(np, "tx-delay", &delay_value)) {
>   cvmx_write_csr(CVMX_ASXX_TX_CLK_SETX(port, iface), delay_value);
> + tx_delay = delay_value > 0;
> + }
> +
> + if (!rx_delay && !tx_delay)
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII_ID;
> + else if (!rx_delay)
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII_RXID;
> + else if (!tx_delay)
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII_TXID;
> + else
> + priv->phy_mode = PHY_INTERFACE_MODE_RGMII;

Humm, that is unique, as far as i know. Every other MAC driver uses
of_get_phy_mode() to get the value out of device tree. The proprietary
delay values can then be used to fine tune the basic delay setting
read from DT.

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

Re: [PATCH v3 00/26] Add definition for the number of standard PCI BARs

[Andrew Murray]

On Mon, Sep 16, 2019 at 11:41:32PM +0300, Denis Efremov wrote:
> Code that iterates over all standard PCI BARs typically uses
> PCI_STD_RESOURCE_END, but this is error-prone because it requires
> "i <= PCI_STD_RESOURCE_END" rather than something like
> "i < PCI_STD_NUM_BARS". We could add such a definition and use it the same
> way PCI_SRIOV_NUM_BARS is used. The patchset also replaces constant (6)
> with new define PCI_STD_NUM_BARS where appropriate and removes local
> declarations for the number of PCI BARs.

I've made several comments to various patches, however for all the patches
you can add:

Reviewed-by: Andrew Murray 

> 
> Changes in v3:
>   - Updated commits description.
>   - Refactored "< PCI_ROM_RESOURCE" with "< PCI_STD_NUM_BARS" in loops.
>   - Refactored "<= BAR_5" with "< PCI_STD_NUM_BARS" in loops.
>   - Removed local define GASKET_NUM_BARS.
>   - Removed local define PCI_NUM_BAR_RESOURCES.
> 
> Changes in v2:
>   - Reversed checks in pci_iomap_range,pci_iomap_wc_range.
>   - Refactored loops in vfio_pci to keep PCI_STD_RESOURCES.
>   - Added 2 new patches to replace the magic constant with new define.
>   - Splitted net patch in v1 to separate stmmac and dwc-xlgmac patches.
> 
> Denis Efremov (26):
>   PCI: Add define for the number of standard PCI BARs
>   PCI: hv: Use PCI_STD_NUM_BARS
>   PCI: dwc: Use PCI_STD_NUM_BARS
>   PCI: endpoint: Use PCI_STD_NUM_BARS
>   misc: pci_endpoint_test: Use PCI_STD_NUM_BARS
>   s390/pci: Use PCI_STD_NUM_BARS
>   x86/PCI: Loop using PCI_STD_NUM_BARS
>   alpha/PCI: Use PCI_STD_NUM_BARS
>   ia64: Use PCI_STD_NUM_BARS
>   stmmac: pci: Loop using PCI_STD_NUM_BARS
>   net: dwc-xlgmac: Loop using PCI_STD_NUM_BARS
>   ixgb: use PCI_STD_NUM_BARS
>   e1000: Use PCI_STD_NUM_BARS
>   rapidio/tsi721: Loop using PCI_STD_NUM_BARS
>   efifb: Loop using PCI_STD_NUM_BARS
>   fbmem: use PCI_STD_NUM_BARS
>   vfio_pci: Loop using PCI_STD_NUM_BARS
>   scsi: pm80xx: Use PCI_STD_NUM_BARS
>   ata: sata_nv: Use PCI_STD_NUM_BARS
>   staging: gasket: Use PCI_STD_NUM_BARS
>   serial: 8250_pci: Use PCI_STD_NUM_BARS
>   pata_atp867x: Use PCI_STD_NUM_BARS
>   memstick: use PCI_STD_NUM_BARS
>   USB: core: Use PCI_STD_NUM_BARS
>   usb: pci-quirks: Use PCI_STD_NUM_BARS
>   devres: use PCI_STD_NUM_BARS
> 
>  arch/alpha/kernel/pci-sysfs.c |  8 ++---
>  arch/ia64/sn/pci/pcibr/pcibr_dma.c|  4 +--
>  arch/s390/include/asm/pci.h   |  5 +--
>  arch/s390/include/asm/pci_clp.h   |  6 ++--
>  arch/s390/pci/pci.c   | 16 +-
>  arch/s390/pci/pci_clp.c   |  6 ++--
>  arch/x86/pci/common.c |  2 +-
>  arch/x86/pci/intel_mid_pci.c  |  2 +-
>  drivers/ata/pata_atp867x.c|  2 +-
>  drivers/ata/sata_nv.c |  2 +-
>  drivers/memstick/host/jmb38x_ms.c |  2 +-
>  drivers/misc/pci_endpoint_test.c  |  8 ++---
>  drivers/net/ethernet/intel/e1000/e1000.h  |  1 -
>  drivers/net/ethernet/intel/e1000/e1000_main.c |  2 +-
>  drivers/net/ethernet/intel/ixgb/ixgb.h|  1 -
>  drivers/net/ethernet/intel/ixgb/ixgb_main.c   |  2 +-
>  .../net/ethernet/stmicro/stmmac/stmmac_pci.c  |  4 +--
>  .../net/ethernet/synopsys/dwc-xlgmac-pci.c|  2 +-
>  drivers/pci/controller/dwc/pci-dra7xx.c   |  2 +-
>  .../pci/controller/dwc/pci-layerscape-ep.c|  2 +-
>  drivers/pci/controller/dwc/pcie-artpec6.c |  2 +-
>  .../pci/controller/dwc/pcie-designware-plat.c |  2 +-
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +-
>  drivers/pci/controller/pci-hyperv.c   | 10 +++---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 10 +++---
>  drivers/pci/pci-sysfs.c   |  4 +--
>  drivers/pci/pci.c | 13 
>  drivers/pci/proc.c|  4 +--
>  drivers/pci/quirks.c  |  4 +--
>  drivers/rapidio/devices/tsi721.c  |  2 +-
>  drivers/scsi/pm8001/pm8001_hwi.c  |  2 +-
>  drivers/scsi/pm8001/pm8001_init.c |  2 +-
>  drivers/staging/gasket/gasket_constants.h |  3 --
>  drivers/staging/gasket/gasket_core.c  | 12 +++
>  drivers/staging/gasket/gasket_core.h  |  4 +--
>  drivers/tty/serial/8250/8250_pci.c|  8 ++---
>  drivers/usb/core/hcd-pci.c|  2 +-
>  drivers/usb/host/pci-quirks.c |  2 +-
>  drivers/vfio/pci/vfio_pci.c   | 11 ---
>  drivers/vfio/pci/vfio_pci_config.c| 32 ++-
>  dr

Re: [PATCH 02/20] staging: wfx: add support for I/O access

[Andrew Lunn]

On Thu, Sep 19, 2019 at 10:52:35AM +, Jerome Pouiller wrote:
> +static int wfx_sdio_copy_from_io(void *priv, unsigned int reg_id,
> +  void *dst, size_t count)
> +{
> + struct wfx_sdio_priv *bus = priv;
> + unsigned int sdio_addr = reg_id << 2;
> + int ret;
> +
> + BUG_ON(reg_id > 7);

Hi Jerome

BUG_ON should only be used when the system is corrupted, and there is
no alternative than to stop the machine, so it does not further
corrupt itself. Accessing a register which does not exist is not a
reason the kill the machine. A WARN() and a return of -EINVAL would be
better.

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

Re: [PATCH v1] staging: intel-dpa: gswip: Introduce Gigabit Ethernet Switch (GSWIP) device driver

[Andrew Lunn]

On Mon, Nov 04, 2019 at 01:20:09PM +0100, Greg KH wrote:
> On Mon, Nov 04, 2019 at 07:22:20PM +0800, Jack Ping CHNG wrote:
> > This driver enables the Intel's LGM SoC GSWIP block.
> > GSWIP is a core module tailored for L2/L3/L4+ data plane and QoS functions.
> > It allows CPUs and other accelerators connected to the SoC datapath
> > to enqueue and dequeue packets through DMAs.
> > Most configuration values are stored in tables such as
> > Parsing and Classification Engine tables, Buffer Manager tables and
> > Pseudo MAC tables.
> 
> Why is this being submitted to staging?  What is wrong with the "real"
> part of the kernel for this?

Or even, what is wrong with the current driver?
drivers/net/dsa/lantiq_gswip.c?

Jack, your patch does not seem to of made it to any of the lists. So i
cannot comment on it contents. If this is a switch driver, please
ensure you Cc: the usual suspects for switch drivers.

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

Re: [PATCH v2] android: binder: Drop lru lock in isolate callback

[Andrew Morton]

On Thu, 14 Sep 2017 14:22:25 -0400 Sherry Yang  wrote:

> Drop the global lru lock in isolate callback
> before calling zap_page_range which calls
> cond_resched, and re-acquire the global lru
> lock before returning. Also change return
> code to LRU_REMOVED_RETRY.
> 
> Use mmput_async when fail to acquire mmap sem
> in an atomic context.
> 
> Fix "BUG: sleeping function called from invalid context"
> errors when CONFIG_DEBUG_ATOMIC_SLEEP is enabled.
> 
> Also restore mmput_async, which was initially introduced in
> ec8d7c14e ("mm, oom_reaper: do not mmput synchronously from
> the oom reaper context"), and was removed in 212925802
> ("mm: oom: let oom_reap_task and exit_mmap run concurrently").

Ho hum, OK.  It's a bit sad to bring this back for one caller but
mm.async_put_work is there and won't be going away.

The mmdrop_async stuff should be moved into fork.c (and maybe made
static) as well.  It's pointless having this stuff global and inlined,
especially mmdrop_async_fn().


Something like the below, not yet tested...


From: Andrew Morton 
Subject: include/linux/sched/mm.h: uninline mmdrop_async(), etc

mmdrop_async() is only used in fork.c.  Move that and its support functions
into fork.c, uninline it all.



Signed-off-by: Andrew Morton 
---

 include/linux/sched/mm.h |   24 +--
 kernel/fork.c|   58 +
 2 files changed, 42 insertions(+), 40 deletions(-)

diff -puN include/linux/sched/mm.h~a include/linux/sched/mm.h
--- a/include/linux/sched/mm.h~a
+++ a/include/linux/sched/mm.h
@@ -10,7 +10,7 @@
 /*
  * Routines for handling mm_structs
  */
-extern struct mm_struct * mm_alloc(void);
+extern struct mm_struct *mm_alloc(void);
 
 /**
  * mmgrab() - Pin a &struct mm_struct.
@@ -34,27 +34,7 @@ static inline void mmgrab(struct mm_stru
atomic_inc(&mm->mm_count);
 }
 
-/* mmdrop drops the mm and the page tables */
-extern void __mmdrop(struct mm_struct *);
-static inline void mmdrop(struct mm_struct *mm)
-{
-   if (unlikely(atomic_dec_and_test(&mm->mm_count)))
-   __mmdrop(mm);
-}
-
-static inline void mmdrop_async_fn(struct work_struct *work)
-{
-   struct mm_struct *mm = container_of(work, struct mm_struct, 
async_put_work);
-   __mmdrop(mm);
-}
-
-static inline void mmdrop_async(struct mm_struct *mm)
-{
-   if (unlikely(atomic_dec_and_test(&mm->mm_count))) {
-   INIT_WORK(&mm->async_put_work, mmdrop_async_fn);
-   schedule_work(&mm->async_put_work);
-   }
-}
+extern void mmdrop(struct mm_struct *mm);
 
 /**
  * mmget() - Pin the address space associated with a &struct mm_struct.
diff -puN kernel/fork.c~a kernel/fork.c
--- a/kernel/fork.c~a
+++ a/kernel/fork.c
@@ -386,6 +386,46 @@ void free_task(struct task_struct *tsk)
 }
 EXPORT_SYMBOL(free_task);
 
+/*
+ * Called when the last reference to the mm
+ * is dropped: either by a lazy thread or by
+ * mmput. Free the page directory and the mm.
+ */
+static void __mmdrop(struct mm_struct *mm)
+{
+   BUG_ON(mm == &init_mm);
+   mm_free_pgd(mm);
+   destroy_context(mm);
+   hmm_mm_destroy(mm);
+   mmu_notifier_mm_destroy(mm);
+   check_mm(mm);
+   put_user_ns(mm->user_ns);
+   free_mm(mm);
+}
+
+void mmdrop(struct mm_struct *mm)
+{
+   if (unlikely(atomic_dec_and_test(&mm->mm_count)))
+   __mmdrop(mm);
+}
+EXPORT_SYMBOL_GPL(mmdrop)
+
+static void mmdrop_async_fn(struct work_struct *work)
+{
+   struct mm_struct *mm;
+
+   mm = container_of(work, struct mm_struct, async_put_work);
+   __mmdrop(mm);
+}
+
+static void mmdrop_async(struct mm_struct *mm)
+{
+   if (unlikely(atomic_dec_and_test(&mm->mm_count))) {
+   INIT_WORK(&mm->async_put_work, mmdrop_async_fn);
+   schedule_work(&mm->async_put_work);
+   }
+}
+
 static inline void free_signal_struct(struct signal_struct *sig)
 {
taskstats_tgid_free(sig);
@@ -895,24 +935,6 @@ struct mm_struct *mm_alloc(void)
return mm_init(mm, current, current_user_ns());
 }
 
-/*
- * Called when the last reference to the mm
- * is dropped: either by a lazy thread or by
- * mmput. Free the page directory and the mm.
- */
-void __mmdrop(struct mm_struct *mm)
-{
-   BUG_ON(mm == &init_mm);
-   mm_free_pgd(mm);
-   destroy_context(mm);
-   hmm_mm_destroy(mm);
-   mmu_notifier_mm_destroy(mm);
-   check_mm(mm);
-   put_user_ns(mm->user_ns);
-   free_mm(mm);
-}
-EXPORT_SYMBOL_GPL(__mmdrop);
-
 static inline void __mmput(struct mm_struct *mm)
 {
VM_BUG_ON(atomic_read(&mm->mm_users));
_

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

Re: [PATCH 0/6] staging: Introduce DPAA2 Ethernet Switch driver

[Andrew Lunn]

On Tue, Sep 19, 2017 at 12:01:32PM +0300, Razvan Stefanescu wrote:
> This patchset introduces the Ethernet Switch Driver for Freescale/NXP SoCs
> with DPAA2 (DataPath Acceleration Architecture v2). The driver manages
> switch objects discovered on the fsl-mc bus. A description of the driver
> can be found in the associated README file.

Hi Razvan

You should probably Cc: netdev with these patches, if you want people
who have written switch drivers to review you code. You can also drop
linux-arm-kernel, since there is nothing ARM specific in these
patches.

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

Re: [PATCH 5/6] staging: fsl-dpaa2/ethsw: Add README

[Andrew Lunn]

On Tue, Sep 19, 2017 at 12:01:37PM +0300, Razvan Stefanescu wrote:
> +Driver uses the switch device driver model and exposes each switch port as
> +a network interface, which can be included in a bridge. Traffic switched
> +between ports is offloaded into the hardware. Exposed network interfaces
> +are not used for I/O, they are used just for configuration. This
> +limitation is going to be addressed in the future.

Hi Razvan

Could you briefly describe how Ethernet frames get from the CPU to the
switch. This is what decided if you should write a plain switchdev
driver, or a DSA driver.

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

Re: [PATCH v2 3/6] staging: fsl-dpaa2/ethsw: Add ethtool support

[Andrew Lunn]

Hi Razvan

> +static void ethsw_get_drvinfo(struct net_device *netdev,
> +   struct ethtool_drvinfo *drvinfo)
> +{
> + struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> + u16 version_major, version_minor;
> + int err;
> +
> + strlcpy(drvinfo->driver, KBUILD_MODNAME, sizeof(drvinfo->driver));
> + strlcpy(drvinfo->version, ethsw_drv_version, sizeof(drvinfo->version));

Software driver versions are mostly useless. I would suggest you
remove this.

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

Re: [PATCH 5/6] staging: fsl-dpaa2/ethsw: Add README

[Andrew Lunn]

On Tue, Oct 03, 2017 at 10:07:41AM +, Razvan Stefanescu wrote:
> > -Original Message-
> > From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org]
> > On Behalf Of Andrew Lunn
> > Sent: Tuesday, September 19, 2017 3:18 PM
> > To: Razvan Stefanescu 
> > Cc: de...@driverdev.osuosl.org; Ruxandra Ioana Radulescu
> > ; a...@arndb.de; gre...@linuxfoundation.org;
> > Alexandru Marginean ; linux-
> > ker...@vger.kernel.org; ag...@suse.de; stuyo...@gmail.com; Bogdan
> > Purcareata ; linux-arm-
> > ker...@lists.infradead.org; Laurentiu Tudor 
> > Subject: Re: [PATCH 5/6] staging: fsl-dpaa2/ethsw: Add README
> > 
> > On Tue, Sep 19, 2017 at 12:01:37PM +0300, Razvan Stefanescu wrote:
> > > +Driver uses the switch device driver model and exposes each switch port 
> > > as
> > > +a network interface, which can be included in a bridge. Traffic switched
> > > +between ports is offloaded into the hardware. Exposed network interfaces
> > > +are not used for I/O, they are used just for configuration. This
> > > +limitation is going to be addressed in the future.
> > 
> > Hi Razvan
> > 
> > Could you briefly describe how Ethernet frames get from the CPU to the
> > switch. This is what decided if you should write a plain switchdev
> > driver, or a DSA driver.
> > 
> > Andrew
> > 
> Hello Andrew,
> 
> CPU frame handling will be added in a later. Each netdevice associated 
> to a switch port will have I/O capabilities like dpaa2-ethernet devices.
> The dpaa2-ethsw will use ACLs to redirect specific types of frames
> (i.e BPDUs) to CPU.

Hi Razvan

I looked at the architecture documentation after i posted this
email. It looks like each switch port will get its own DMA queues, etc
on the host. It is not sharing one host interface to get packets to
the switch, which is what DSA does. So a pure switchdev driver is the
correct solution here.

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

Re: [PATCH v3 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

> + priv->phy_np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> + priv->phy_mode = of_get_phy_mode(pdev->dev.of_node);
> + /* If phy-mode absent, default to SGMII. */
> + if (priv->phy_mode < 0)
> + priv->phy_mode = PHY_INTERFACE_MODE_SGMII;
> +
> + if (priv->phy_mode == PHY_INTERFACE_MODE_1000BASEX)
> + priv->mode_1000basex = true;
> +
> + if (of_phy_is_fixed_link(pdev->dev.of_node))
> + priv->bgx_as_phy = true;
> +

...

> + priv->mode = bgx_port_get_mode(priv->node, priv->bgx, priv->index);
> +

It might be a good idea to verify priv->phy_mode and priv->mode are
compatible.

> + switch (priv->mode) {
> + case PORT_MODE_SGMII:
> + case PORT_MODE_RGMII:
> + priv->get_link = bgx_port_get_sgmii_link;
> + priv->set_link = bgx_port_set_xgmii_link;
> + break;
> + case PORT_MODE_XAUI:
> + case PORT_MODE_RXAUI:
> + case PORT_MODE_XLAUI:
> + case PORT_MODE_XFI:
> + case PORT_MODE_10G_KR:
> + case PORT_MODE_40G_KR4:
> + priv->get_link = bgx_port_get_xaui_link;
> + priv->set_link = bgx_port_set_xaui_link;
> + break;


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

Re: [PATCH v3 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

> + if (link_changed != 0) {
> + struct port_status status;
> +
> + if (link_changed > 0) {
> + netdev_info(netdev, "Link is up - %d/%s\n",
> + priv->phydev->speed,
> + priv->phydev->duplex == DUPLEX_FULL ?
> + "Full" : "Half");
> + } else {
> + netdev_info(netdev, "Link is down\n");
> + }

phy_print_status()

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

Re: [PATCH v3] staging: fsl-mc: move bus driver out of staging

[Andrew Lunn]

On Tue, Nov 28, 2017 at 05:27:57PM +0200, laurentiu.tu...@nxp.com wrote:
> diff --git a/drivers/staging/fsl-mc/bus/dpmcp.h b/drivers/bus/fsl-mc/dpmcp.h
> similarity index 100%
> rename from drivers/staging/fsl-mc/bus/dpmcp.h
> rename to drivers/bus/fsl-mc/dpmcp.h
> diff --git a/drivers/staging/fsl-mc/bus/dpmng-cmd.h 
> b/drivers/bus/fsl-mc/dpmng-cmd.h
> similarity index 100%
> rename from drivers/staging/fsl-mc/bus/dpmng-cmd.h
> rename to drivers/bus/fsl-mc/dpmng-cmd.h
> diff --git a/drivers/staging/fsl-mc/bus/dprc-cmd.h 
> b/drivers/bus/fsl-mc/dprc-cmd.h
> similarity index 100%
> rename from drivers/staging/fsl-mc/bus/dprc-cmd.h
> rename to drivers/bus/fsl-mc/dprc-cmd.h
> diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c 
> b/drivers/bus/fsl-mc/dprc-driver.c
> similarity index 99%
> rename from drivers/staging/fsl-mc/bus/dprc-driver.c
> rename to drivers/bus/fsl-mc/dprc-driver.c

Hi Laurentiu

It is hard to review code when we don't see it. 

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

Re: [PATCH v4 1/8] dt-bindings: Add Cavium Octeon Common Ethernet Interface.

[Andrew Lunn]

On Tue, Nov 28, 2017 at 04:55:33PM -0800, David Daney wrote:
> From: Carlos Munoz 
> 
> Add bindings for Common Ethernet Interface (BGX) block.
> 
> Acked-by: Rob Herring 
> Signed-off-by: Carlos Munoz 
> Signed-off-by: Steven J. Hill 
> Signed-off-by: David Daney 
> ---
>  .../devicetree/bindings/net/cavium-bgx.txt | 61 
> ++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/cavium-bgx.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/cavium-bgx.txt 
> b/Documentation/devicetree/bindings/net/cavium-bgx.txt
> new file mode 100644
> index ..830c5f08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/cavium-bgx.txt
> @@ -0,0 +1,61 @@
> +* Common Ethernet Interface (BGX) block
> +
> +Properties:
> +
> +- compatible: "cavium,octeon-7890-bgx": Compatibility with all cn7xxx SOCs.
> +
> +- reg: The base address of the BGX block.
> +
> +- #address-cells: Must be <1>.
> +
> +- #size-cells: Must be <0>.  BGX addresses have no size component.
> +
> +A BGX block has several children, each representing an Ethernet
> +interface.
> +
> +
> +* Ethernet Interface (BGX port) connects to PKI/PKO
> +
> +Properties:
> +
> +- compatible: "cavium,octeon-7890-bgx-port": Compatibility with all
> +   cn7xxx SOCs.
> +
> +   "cavium,octeon-7360-xcv": Compatibility with cn73xx SOCs
> +   for RGMII.
> +
> +- reg: The index of the interface within the BGX block.
> +
> +Optional properties:
> +
> +- local-mac-address: Mac address for the interface.
> +
> +- phy-handle: phandle to the phy node connected to the interface.
> +
> +- phy-mode: described in ethernet.txt.
> +
> +- fixed-link: described in fixed-link.txt.
> +
> +Example:
> +
> + ethernet-mac-nexus@11800e000 {
> + compatible = "cavium,octeon-7890-bgx";
> + reg = <0x00011800 0xe000 0x 0x0100>;

Hi David

In the probe function we have:

+   reg = of_get_property(pdev->dev.of_node, "reg", NULL);
+   addr = of_translate_address(pdev->dev.of_node, reg);
+   interface = (addr >> 24) & 0xf;
+   numa_node = (addr >> 36) & 0x7;

Is this documented somewhere? The numa_node is particularly
interesting. MMIO changes depends on what node this is in the cluster?

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

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

On Wed, Nov 29, 2017 at 04:00:01PM +0530, Souptick Joarder wrote:

Hi Souptick

Please trim the code when giving reviews. We don't want to have to
page through 8K lines of code it find a few comments mixed in. Just
keep the beginning of the function you are commented on to make the
context clear. Cut the rest.

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

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

On Wed, Nov 29, 2017 at 10:11:38PM +0300, Dan Carpenter wrote:
> On Wed, Nov 29, 2017 at 09:37:15PM +0530, Souptick Joarder wrote:
> > >> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv, 
> > >> struct port_status status)
> > >> +{
> > >> +   u64 data;
> > >> +   u64 prtx;
> > >> +   u64 miscx;
> > >> +   int timeout;
> > >> +
> > 
> > >> +
> > >> +   switch (status.speed) {
> > >> +   case 10:
> > >
> > > In my opinion, instead of hard coding the value, is it fine to use ENUM ?
> >Similar comments applicable in other places where hard coded values are 
> > used.
> > 
> 
> 10 means 10M right?  That's not really a magic number.  It's fine.

There are also :
uapi/linux/ethtool.h:#define SPEED_10   10
uapi/linux/ethtool.h:#define SPEED_100      100
uapi/linux/ethtool.h:#define SPEED_1000 1000
uapi/linux/ethtool.h:#define SPEED_11
uapi/linux/ethtool.h:#define SPEED_10   10

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

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn]

tus.duplex = priv->phydev->duplex;
> +
> + mutex_unlock(&priv->lock);
> +
> + if (link_changed) {
> + struct port_status status;
> +
> + phy_print_status(priv->phydev);
> +
> + status.link = link ? 1 : 0;
> + status.duplex = duplex;
> + status.speed = speed;
> + if (!link) {
> + netif_carrier_off(netdev);
> +  /* Let TX drain. FIXME check that it is drained. */
> + mdelay(50);
> + }
> + priv->set_link(priv, status);
> + if (link)
> + netif_carrier_on(netdev);

The code should do netif_carrier_on/off for you. See phy_link_change()

> +static void bgx_port_check_state(struct work_struct *work)
> +{
> + struct bgx_port_priv*priv;
> + struct port_status  status;
> +
> + priv = container_of(work, struct bgx_port_priv, dwork.work);
> +
> + status = priv->get_link(priv);
> +
> + if (!status.link &&
> + priv->mode != PORT_MODE_SGMII && priv->mode != PORT_MODE_RGMII)
> + bgx_port_init_xaui_link(priv);
> +
> + if (priv->last_status.link != status.link) {
> + priv->last_status.link = status.link;
> + if (status.link)
> + netdev_info(priv->netdev, "Link is up - %d/%s\n",
> + status.speed,
> + status.duplex == DUPLEX_FULL ? "Full" : 
> "Half");

You already have phy_print_status() in bgx_port_adjust_link(). Do you need this 
here?

> + else
> + netdev_info(priv->netdev, "Link is down\n");
> + }
> +
> + mutex_lock(&priv->lock);
> + if (priv->work_queued)
> + queue_delayed_work(check_state_wq, &priv->dwork, HZ);
> + mutex_unlock(&priv->lock);
> +}
> +
> +int bgx_port_enable(struct net_device *netdev)
> +{


> + } else {
> + priv->phydev = of_phy_connect(netdev, priv->phy_np,
> +   bgx_port_adjust_link, 0, 
> priv->phy_mode);
> + if (!priv->phydev)
> + return -ENODEV;
> +
> + netif_carrier_off(netdev);
> +
> + if (priv->phydev)

You already checked this above.

> + phy_start_aneg(priv->phydev);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(bgx_port_enable);
> +
> +int bgx_port_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> + struct bgx_port_priv *priv = bgx_port_netdev2priv(netdev);
> + int max_frame;
> +
> + if (new_mtu < 60 || new_mtu > 65392) {

See dev_set_mtu(). If you have done your initialisation correctly, this
won't happen.

> +static int bgx_port_probe(struct platform_device *pdev)
> +{
> + switch (priv->mode) {
> + case PORT_MODE_SGMII:
> + if (priv->phy_np &&
> + priv->phy_mode != PHY_INTERFACE_MODE_SGMII)
> + dev_warn(&pdev->dev, "SGMII phy mode mismatch.\n");
> + goto set_link_functions;
> + case PORT_MODE_RGMII:
> + if (priv->phy_np &&
> + priv->phy_mode != PHY_INTERFACE_MODE_RGMII &&
> + priv->phy_mode != PHY_INTERFACE_MODE_RGMII_ID &&
> + priv->phy_mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> + priv->phy_mode != PHY_INTERFACE_MODE_RGMII_TXID)

phy_interface_mode_is_rgmii()

More later, maybe.

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

Re: [PATCH v7 4/4] kexec_file: Load kernel at top of system RAM if required

[Andrew Morton]

On Wed, 18 Jul 2018 10:49:44 +0800 Baoquan He  wrote:

> For kexec_file loading, if kexec_buf.top_down is 'true', the memory which
> is used to load kernel/initrd/purgatory is supposed to be allocated from
> top to down. This is what we have been doing all along in the old kexec
> loading interface and the kexec loading is still default setting in some
> distributions. However, the current kexec_file loading interface doesn't
> do like this. The function arch_kexec_walk_mem() it calls ignores checking
> kexec_buf.top_down, but calls walk_system_ram_res() directly to go through
> all resources of System RAM from bottom to up, to try to find memory region
> which can contain the specific kexec buffer, then call 
> locate_mem_hole_callback()
> to allocate memory in that found memory region from top to down. This brings
> confusion especially when KASLR is widely supported , users have to make clear
> why kexec/kdump kernel loading position is different between these two
> interfaces in order to exclude unnecessary noises. Hence these two interfaces
> need be unified on behaviour.

As far as I can tell, the above is the whole reason for the patchset,
yes?  To avoid confusing users.

Is that sufficient?  Can we instead simplify their lives by providing
better documentation or informative printks or better Kconfig text,
etc?

And who *are* the people who are performing this configuration?  Random
system administrators?  Linux distro engineers?  If the latter then
they presumably aren't easily confused!

In other words, I'm trying to understand how much benefit this patchset
will provide to our users as a whole.

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

Re: [PATCH v7 4/4] kexec_file: Load kernel at top of system RAM if required

[Andrew Morton]

On Thu, 19 Jul 2018 23:17:53 +0800 Baoquan He  wrote:

> Hi Andrew,
> 
> On 07/18/18 at 03:33pm, Andrew Morton wrote:
> > On Wed, 18 Jul 2018 10:49:44 +0800 Baoquan He  wrote:
> > 
> > > For kexec_file loading, if kexec_buf.top_down is 'true', the memory which
> > > is used to load kernel/initrd/purgatory is supposed to be allocated from
> > > top to down. This is what we have been doing all along in the old kexec
> > > loading interface and the kexec loading is still default setting in some
> > > distributions. However, the current kexec_file loading interface doesn't
> > > do like this. The function arch_kexec_walk_mem() it calls ignores checking
> > > kexec_buf.top_down, but calls walk_system_ram_res() directly to go through
> > > all resources of System RAM from bottom to up, to try to find memory 
> > > region
> > > which can contain the specific kexec buffer, then call 
> > > locate_mem_hole_callback()
> > > to allocate memory in that found memory region from top to down. This 
> > > brings
> > > confusion especially when KASLR is widely supported , users have to make 
> > > clear
> > > why kexec/kdump kernel loading position is different between these two
> > > interfaces in order to exclude unnecessary noises. Hence these two 
> > > interfaces
> > > need be unified on behaviour.
> > 
> > As far as I can tell, the above is the whole reason for the patchset,
> > yes?  To avoid confusing users.
> 
> 
> In fact, it's not just trying to avoid confusing users. Kexec loading
> and kexec_file loading are just do the same thing in essence. Just we
> need do kernel image verification on uefi system, have to port kexec
> loading code to kernel. 
> 
> Kexec has been a formal feature in our distro, and customers owning
> those kind of very large machine can make use of this feature to speed
> up the reboot process. On uefi machine, the kexec_file loading will
> search place to put kernel under 4G from top to down. As we know, the
> 1st 4G space is DMA32 ZONE, dma, pci mmcfg, bios etc all try to consume
> it. It may have possibility to not be able to find a usable space for
> kernel/initrd. From the top down of the whole memory space, we don't
> have this worry. 
> 
> And at the first post, I just posted below with AKASHI's
> walk_system_ram_res_rev() version. Later you suggested to use
> list_head to link child sibling of resource, see what the code change
> looks like.
> http://lkml.kernel.org/r/20180322033722.9279-1-...@redhat.com
> 
> Then I posted v2
> http://lkml.kernel.org/r/20180408024724.16812-1-...@redhat.com
> Rob Herring mentioned that other components which has this tree struct
> have planned to do the same thing, replacing the singly linked list with
> list_head to link resource child sibling. Just quote Rob's words as
> below. I think this could be another reason.
> 
> ~ From Rob
> The DT struct device_node also has the same tree structure with
> parent, child, sibling pointers and converting to list_head had been
> on the todo list for a while. ACPI also has some tree walking
> functions (drivers/acpi/acpica/pstree.c). Perhaps there should be a
> common tree struct and helpers defined either on top of list_head or a
> ~
> new struct if that saves some size.

Please let's get all this into the changelogs?

> > 
> > Is that sufficient?  Can we instead simplify their lives by providing
> > better documentation or informative printks or better Kconfig text,
> > etc?
> > 
> > And who *are* the people who are performing this configuration?  Random
> > system administrators?  Linux distro engineers?  If the latter then
> > they presumably aren't easily confused!
> 
> Kexec was invented for kernel developer to speed up their kernel
> rebooting. Now high end sever admin, kernel developer and QE are also
> keen to use it to reboot large box for faster feature testing, bug
> debugging. Kernel dev could know this well, about kernel loading
> position, admin or QE might not be aware of it very well. 
> 
> > 
> > In other words, I'm trying to understand how much benefit this patchset
> > will provide to our users as a whole.
> 
> Understood. The list_head replacing patch truly involes too many code
> changes, it's risky. I am willing to try any idea from reviewers, won't
> persuit they have to be accepted finally. If don't have a try, we don't
> know what it looks like, and what impact it may have. I am fine to take
> AKASHI's simple version of walk_system_ram_res_rev() to lower risk, even
> though it could be a little bit low efficient.

The larger patch produces a better result.  We can handle it ;)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH net-next] [RFC] dpaa2-eth: Move DPAA2 Ethernet driver from staging to drivers/net

[Andrew Lunn]

> +static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev)
> +{
> + struct device *dev;
> + struct net_device *net_dev = NULL;
> + struct dpaa2_eth_priv *priv = NULL;
> + int err = 0;
> +
> + dev = &dpni_dev->dev;
> +
> + /* Net device */
> + net_dev = alloc_etherdev_mq(sizeof(*priv), DPAA2_ETH_MAX_TX_QUEUES);
> + if (!net_dev) {
> + dev_err(dev, "alloc_etherdev_mq() failed\n");
> + return -ENOMEM;
> + }
> +
> + SET_NETDEV_DEV(net_dev, dev);
> + dev_set_drvdata(dev, net_dev);
> +
> + priv = netdev_priv(net_dev);
> + priv->net_dev = net_dev;
> +
> + priv->iommu_domain = iommu_get_domain_for_dev(dev);
> +
> + /* Obtain a MC portal */
> + err = fsl_mc_portal_allocate(dpni_dev, FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
> +  &priv->mc_io);
> + if (err) {
> + if (err == -ENXIO)
> + err = -EPROBE_DEFER;
> + else
> + dev_err(dev, "MC portal allocation failed\n");
> + goto err_portal_alloc;
> + }
> +
> + /* MC objects initialization and configuration */
> + err = setup_dpni(dpni_dev);
> + if (err)
> + goto err_dpni_setup;
> +
> + err = setup_dpio(priv);
> + if (err)
> + goto err_dpio_setup;
> +
> + setup_fqs(priv);
> +
> + err = setup_dpbp(priv);
> + if (err)
> + goto err_dpbp_setup;
> +
> + err = bind_dpni(priv);
> + if (err)
> + goto err_bind;
> +
> + /* Add a NAPI context for each channel */
> + add_ch_napi(priv);
> +
> + /* Percpu statistics */
> + priv->percpu_stats = alloc_percpu(*priv->percpu_stats);
> + if (!priv->percpu_stats) {
> + dev_err(dev, "alloc_percpu(percpu_stats) failed\n");
> + err = -ENOMEM;
> + goto err_alloc_percpu_stats;
> + }
> + priv->percpu_extras = alloc_percpu(*priv->percpu_extras);
> + if (!priv->percpu_extras) {
> + dev_err(dev, "alloc_percpu(percpu_extras) failed\n");
> + err = -ENOMEM;
> + goto err_alloc_percpu_extras;
> + }
> +
> + err = netdev_init(net_dev);
> + if (err)
> + goto err_netdev_init;

At the end of netdev_init() you call netdev_register(). From that
point on, you device is live. Its .ndo's can be called

> +
> + /* Configure checksum offload based on current interface flags */
> + err = set_rx_csum(priv, !!(net_dev->features & NETIF_F_RXCSUM));
> + if (err)
> + goto err_csum;
> +
> + err = set_tx_csum(priv, !!(net_dev->features &
> +(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)));
> + if (err)
> + goto err_csum;
> +
> + err = alloc_rings(priv);
> + if (err)
> + goto err_alloc_rings;

How well does the device work if it has not allocated the rings yet,
when it is asked to do something?

> +
> + net_dev->ethtool_ops = &dpaa2_ethtool_ops;
> +
> + err = setup_irqs(dpni_dev);

How well do it work without interrupts being set up?

> + if (err) {
> + netdev_warn(net_dev, "Failed to set link interrupt, fall back 
> to polling\n");
> + priv->poll_thread = kthread_run(poll_link_state, priv,
> + "%s_poll_link", net_dev->name);
> + if (IS_ERR(priv->poll_thread)) {
> + netdev_err(net_dev, "Error starting polling thread\n");
> + goto err_poll_thread;
> + }
> + priv->do_link_poll = true;
> + }

Probably the correct place to register the netdev is here.

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

Re: [PATCH net-next v2 2/2] dpaa2-eth: Move DPAA2 Ethernet driver from staging to drivers/net

[Andrew Lunn]

On Wed, Aug 29, 2018 at 03:50:02AM -0700, Joe Perches wrote:
> On Wed, 2018-08-29 at 04:42 -0500, Ioana Radulescu wrote:
> > The DPAA2 Ethernet driver supports Freescale/NXP SoCs with DPAA2
> > (DataPath Acceleration Architecture v2). The driver manages
> > network objects discovered on the fsl-mc bus.
> 
> Please use git 'format-patch -M' to make the diff
> smaller and more readable.

Hi Joe

I asked for this.

This is a request to move the driver from staging into the main
tree. We want to review the code in order to see if it has reached
mainline quality.

If the patch just lists a rename, not actually code, i cannot review
it, so i will just NACK it. We need to see the code.

Once the code has been reviewed and has all the needed Acked-by:, then
-M could be used. But this driver is not that far yet.

Thanks

Andrew


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

Re: simplify procfs code for seq_file instances

[Andrew Morton]

On Tue, 24 Apr 2018 16:23:04 +0200 Christoph Hellwig  wrote:

> On Thu, Apr 19, 2018 at 09:57:50PM +0300, Alexey Dobriyan wrote:
> > > git://git.infradead.org/users/hch/misc.git proc_create
> > 
> > 
> > I want to ask if it is time to start using poorman function overloading
> > with _b_c_e(). There are millions of allocation functions for example,
> > all slightly difference, and people will add more. Seeing /proc interfaces
> > doubled like this is painful.
> 
> Function overloading is totally unacceptable.
> 
> And I very much disagree with a tradeoff that keeps 5000 lines of 
> code vs a few new helpers.

OK, the curiosity and suspense are killing me.  What the heck is
"function overloading with _b_c_e()"?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] [RFC] staging/net: move AF_X25 into drivers/staging

[Andrew Lunn]

On Wed, Dec 11, 2019 at 08:10:34AM +0100, Krzysztof Hałasa wrote:
> Arnd,
> 
> Arnd Bergmann  writes:
> 
> > - Most other supported HDLC hardware that we supoprt is for the ISA or
> >   PCI buses.
> 
> I would be surprised if there is anybody left with ISA sync serial
> stuff, but the PCI hardware still has some users - these machines don't
> need to be upgraded yearly. Most people have migrated away, though.

Hi Krzysztof, Arnd

I have a use case for hdlc_cisco and hdlc_raw_eth. But it uses a lot
of out of tree code, the DAHDI driver framework for E1 cards, and an
E1 card which is not part of DAHDI.

Given how much of this is out of tree, i would understand if you
eventually decide to remove this HDLC code.

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

Re: FW: [PATCH v2] staging: intel-gwdpa: gswip: Introduce Gigabit Ethernet Switch (GSWIP) device driver

[Andrew Lunn]

> > We are trying to upstream the datapath code for Intel new NoC gateway
> > (please refer to intel-gwdpa.txt at the end of the patch). It consists of
> > ethernet, WIFI and passive optics handling. Since the code is quite huge, we
> > have broken it into parts for internal review.
> > 
> > As we have seen past upstream example such as fsl/dpaa, we thought that it
> > is better for us to start the upstreaming of the driver into staging folder
> > to get feedback from the community.
> > 
> > Is this the right approach? Or do we upstream all the drivers into
> > drivers/soc folder when we have all the drivers ready?
> 
> Why is drivers/soc/ the place to put networking drivers?
> 
> Please please please work with the Intel Linux kernel developers who
> know how to do this type of thing and do not require the kernel
> community to teach you all the proper development model and methods
> here.

I see a lot in common with dpaa2 here. You have a non traditional
hardware architecture. That means it does not nicely fit into the tree
as other drivers do.

There also appears to of been a huge amount of code development behind
closed doors, same as dpaa2. And because of the non traditional
architecture, you have had to make all sorts of design decisions. And
because that happened behind closed door, i'm sure some are
wrong. dpaa2 has been in staging for around 2 1/2 years now. It takes
that amount of time to discuss how non traditional hardware should be
supported in Linux, an re-write the drivers as needed because of the
wrong design decisions.

I kind of expect you are going to have a similar experience. So as
well as getting the Intel Linux kernel developers involved for process
and architecture support, you might want to look at how the dpaa2
drivers have evolved, what they got wrong, what they got right. How is
your hardware similar and different. And look at what parts of dpaa2
have moved out of staging, and maybe consider that code as a good
model to follow.

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

Re: [PATCH] staging: usbip: userspace: increase version to 2.0

[Andrew Grover]

On Wed, Mar 5, 2014 at 10:14 PM, Valentina Manea
 wrote:
> On Tue, Mar 4, 2014 at 9:20 PM, Greg KH  wrote:
>> On Tue, Mar 04, 2014 at 09:16:39PM +0200, Valentina Manea wrote:
>>> -AC_INIT([usbip-utils], [1.1.1], [linux-...@vger.kernel.org])
>>> +AC_INIT([usbip-utils], [2.0], [linux-...@vger.kernel.org])
>>
>> Why?
>>
>> What does this mean?  What warrents the version change?  Why have a
>> version at all?
>
> This was part of an effort to "refresh" USB/IP by moving userspace out
> of kernel.git.
> Since some major changes have been made (libudev migration), Andy (cc'ed) and 
> me
> thought it was worth to be promoted to version 2.0.

(sorry, resending in plain text mode so vger doesn't bounce (curse you
gmail :-))

Valentina did considerable work in moving usbip-utils from using the
defunct libsysfs to libudev (well, part of systemd now it seems.) so
some version bump seems appropriate, why not to 2.0? esp. as a
heads-up to pkg maintainers - btw usbip-utils is already packaged for
Debian, and I could probably see it in Fedora too, why not.

As to why have a version at all, this is of course tied to whether
usbip-utils will ever emerge from the belly of the whale and return to
its own home. I think it should someday, if the concerns about
long-term maintenance and interface stability can be addressed to your
satisfaction. It would be considerable work to integrate it into the
kernel build, and would need to be undone if it ever left kernel.git.

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

Re: [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration

[Andrew Morton]

On Thu, 06 Mar 2014 15:28:40 -0800 Joe Perches  wrote:

> Networking prefers this style, so warn when it's not used.
> 
> Networking uses:
> 
> void foo(int bar)
> {
>   int baz;
> 
>   code...
> }
> 
> not
> 
> void foo(int bar)
> {
>   int baz;
>   code...
> }
> 
> There are a limited number of false positives when using
> macros to declare variables like:
> 
> WARNING: networking uses a blank line after declarations
> #330: FILE: net/ipv4/inet_hashtables.c:330:
> + int dif = sk->sk_bound_dev_if;
> + INET_ADDR_COOKIE(acookie, saddr, daddr)

um wait wut wot.

*All* kernel code uses blank line between end-of-locals and
start-of-code.  Or if it doesn't it should, thwap.

Why are we special-casing net/?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration

[Andrew Morton]

On Thu, 06 Mar 2014 15:42:30 -0800 Joe Perches  wrote:

> On Thu, 2014-03-06 at 15:35 -0800, Andrew Morton wrote:
> > On Thu, 06 Mar 2014 15:28:40 -0800 Joe Perches  wrote:
> > > Networking prefers this style, so warn when it's not used.
> > > void foo(int bar)
> > > {
> > >   int baz;
> > > 
> > >   code...
> > > }
> > > 
> > > not
> > > 
> > > void foo(int bar)
> > > {
> > >   int baz;
> > >   code...
> > > }
> > > 
> > > There are a limited number of false positives when using
> > > macros to declare variables like:
> > > 
> > > WARNING: networking uses a blank line after declarations
> > > #330: FILE: net/ipv4/inet_hashtables.c:330:
> > > + int dif = sk->sk_bound_dev_if;
> > > + INET_ADDR_COOKIE(acookie, saddr, daddr)
> > 
> > um wait wut wot.
> > 
> > *All* kernel code uses blank line between end-of-locals and
> > start-of-code.  Or if it doesn't it should, thwap.
> > Why are we special-casing net/?
> 
> It's easy enough to remove the path test, but it's
> not in CodingStyle and David seems to be the one
> that makes the effort to correct people about it.
> 
> I don't care one way or another.
> 
> I'm just trying to get fewer rejections for people
> that use checkpatch.

mutter.

OK, let's do this for now.  Could you please cook up a followon patch
which makes this kernel-wide?  I'll play with that for a while then I'll
decide how much I feel like irritating people.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/3] memstick: Add realtek USB memstick host driver

[Andrew Morton]

On Wed, 12 Feb 2014 18:00:38 +0800  wrote:

> From: Roger Tseng 
> 
> Realtek USB memstick host driver provides memstick host support based on the
> Realtek USB card reader MFD driver.
> 
> ...
>
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct rtsx_usb_ms {
> + struct platform_device  *pdev;
> + struct rtsx_ucr *ucr;
> + struct memstick_host*msh;
> + struct memstick_request *req;
> +
> + struct mutexhost_mutex;

Should have included mutex.h.

> + struct work_struct  handle_req;
> +
> + struct task_struct  *detect_ms;

sched.h.

> + struct completion   detect_ms_exit;

completion.h.

> + u8  ssc_depth;
> + unsigned intclock;
> + int power_mode;
> + unsigned char   ifmode;
> + booleject;
> +};
> +
> 
> ...
>
> +
> +#else
> +
> +#define ms_print_debug_regs(host)

static void ms_print_debug_regs(struct rtsx_usb_ms *host)
{
}

It is good to have the same signature for either case.  And doing it in
C provides typechecking and can avoid variable-unused warnings.  

> +#endif
> 
> ...
>
> +static int ms_power_on(struct rtsx_usb_ms *host)
> +{
> + struct rtsx_ucr *ucr = host->ucr;
> + int err;
> +
> + dev_dbg(ms_dev(host), "%s\n", __func__);
> +
> + rtsx_usb_init_cmd(ucr);
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, CARD_SELECT, 0x07, MS_MOD_SEL);
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, CARD_SHARE_MODE,
> + CARD_SHARE_MASK, CARD_SHARE_MS);
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, CARD_CLK_EN,
> + MS_CLK_EN, MS_CLK_EN);
> + err = rtsx_usb_send_cmd(ucr, MODE_C, 100);
> + if (err < 0)
> + return err;
> +
> + if (CHECK_PKG(ucr, LQFP48))
> + err = ms_pull_ctl_enable_lqfp48(ucr);
> + else
> + err = ms_pull_ctl_enable_qfn24(ucr);
> + if (err < 0)
> + return err;
> +
> + err = rtsx_usb_write_register(ucr, CARD_PWR_CTL,
> + POWER_MASK, PARTIAL_POWER_ON);
> + if (err)
> + return err;
> +
> + /* Wait ms power stable */

Comment is strange.

> + usleep_range(800, 1000);
> +
> + rtsx_usb_init_cmd(ucr);
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, CARD_PWR_CTL,
> + POWER_MASK, POWER_ON);
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, CARD_OE,
> + MS_OUTPUT_EN, MS_OUTPUT_EN);
> +
> + return rtsx_usb_send_cmd(ucr, MODE_C, 100);
> +}
> +
> 
> ...
>
> +static int ms_transfer_data(struct rtsx_usb_ms *host, unsigned char data_dir,
> + u8 tpc, u8 cfg, struct scatterlist *sg)
> +{
> + struct rtsx_ucr *ucr = host->ucr;
> + int err;
> + unsigned int length = sg->length;
> + u16 sec_cnt = (u16)(length / 512);
> + u8 trans_mode, dma_dir, flag;
> + unsigned int pipe;
> + struct memstick_dev *card = host->msh->card;
> +
> + dev_dbg(ms_dev(host), "%s: tpc = 0x%02x, data_dir = %s, length = %d\n",

length = %u

> + __func__, tpc, (data_dir == READ) ? "READ" : "WRITE",
> + length);
> +
> + if (data_dir == READ) {
> + flag = MODE_CDIR;
> + dma_dir = DMA_DIR_FROM_CARD;
> + if (card->id.type != MEMSTICK_TYPE_PRO)
> + trans_mode = MS_TM_NORMAL_READ;
> + else
> + trans_mode = MS_TM_AUTO_READ;
> + pipe = usb_rcvbulkpipe(ucr->pusb_dev, EP_BULK_IN);
> + } else {
> + flag = MODE_CDOR;
> + dma_dir = DMA_DIR_TO_CARD;
> + if (card->id.type != MEMSTICK_TYPE_PRO)
> + trans_mode = MS_TM_NORMAL_WRITE;
> + else
> + trans_mode = MS_TM_AUTO_WRITE;
> + pipe = usb_sndbulkpipe(ucr->pusb_dev, EP_BULK_OUT);
> + }
> +
> + rtsx_usb_init_cmd(ucr);
> +
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MS_TPC, 0xFF, tpc);
> + if (card->id.type == MEMSTICK_TYPE_PRO) {
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MS_SECTOR_CNT_H,
> + 0xFF, (u8)(sec_cnt >> 8));
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MS_SECTOR_CNT_L,
> + 0xFF, (u8)sec_cnt);
> + }
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MS_TRANS_CFG, 0xFF, cfg);
> +
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MC_DMA_TC3,
> + 0xFF, (u8)(length >> 24));
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MC_DMA_TC2,
> + 0xFF, (u8)(length >> 16));
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MC_DMA_TC1,
> + 0xFF, (u8)(length >> 8));
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MC_DMA_TC0, 0xFF,
> + (u8)length);
> + rtsx_usb_add_cmd(ucr, WRITE_REG_CMD, MC_DMA_CTL,
> + 0x03 | DMA_PACK_

Re: [PATCH v4 3/3] memstick: Add realtek USB memstick host driver

[Andrew Morton]

On Thu, 20 Mar 2014 16:38:03 +0800 Roger  wrote:

> On 02/12/2014 06:00 PM, rogera...@realtek.com wrote:
> > From: Roger Tseng 
> >
> > Realtek USB memstick host driver provides memstick host support based on the
> > Realtek USB card reader MFD driver.
> >
> > Signed-off-by: Roger Tseng 
> Andrew,
> 
> Would you please Ack or comment this patch(3/3) to let the 3 patches be 
> merged together? I have been making the same request at the message 
> thread of "[PATCH v4 2/3]" since several weeks ago but got no response. 
> Thus I re-post here and hope I could get something.
> 

Looks OK to me - I had a few minor quibbles.  Please proceed in that
way.

It would be nice if Alex and/or Maxim had time to look through the code.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC] quiet checkpatch style recommendation about no spaces around bitfield :

[Andrew Morton]

On Mon, 31 Mar 2014 08:31:38 -0700 Joe Perches  wrote:

> > > @@ -143,13 +143,13 @@ union cvmx_usbcx_gahbcfg {
> > >*  * 1'b1: Unmask the interrupt assertion to the application.
> > >*/
> > >   struct cvmx_usbcx_gahbcfg_s {
> > > - uint32_t reserved_9_31  : 23;
> > > - uint32_t ptxfemplvl : 1;
> > > - uint32_t nptxfemplvl: 1;
> > > - uint32_t reserved_6_6   : 1;
> > > - uint32_t dmaen  : 1;
> > > - uint32_t hbstlen: 4;
> > > - uint32_t glblintrmsk: 1;
> > > + uint32_t reserved_9_31:23;
> > > + uint32_t ptxfemplvl:1;
> > > + uint32_t nptxfemplvl:1;
> > > + uint32_t reserved_6_6:1;
> > > + uint32_t dmaen:1;
> > > + uint32_t hbstlen:4;
> > > + uint32_t glblintrmsk:1;
> > >   } s;
> > The warning here is:
> > ERROR: spaces prohibited around that ':' (ctx:WxW)
> > I have done a kernel wide search for these warnings
> 
> Really?  How?
> 
> >  and I think we
> > should disable this warning.  It has too many false positives like this.
> 
> It does seem a bit noisy to me too.
> 
> Andy?  Andrew?

I suspect we don't use bitfields enough for a particular style to have
emerged.  Personally I think the above patch made the code harder to
read, so...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 3/3] memstick: Add realtek USB memstick host driver

[Andrew Morton]

On Tue, 1 Apr 2014 11:20:32 +0800 Roger  wrote:

> On 03/25/2014 06:44 PM, rogera...@realtek.com wrote:
> > From: Roger Tseng 
> >
> > Realtek USB memstick host driver provides memstick host support based on the
> > Realtek USB card reader MFD driver.
> >
> > Signed-off-by: Roger Tseng 
> > ---
> >   drivers/memstick/host/Kconfig   |  10 +
> >   drivers/memstick/host/Makefile  |   1 +
> >   drivers/memstick/host/rtsx_usb_ms.c | 839 
> > 
> >   3 files changed, 850 insertions(+)
> >   create mode 100644 drivers/memstick/host/rtsx_usb_ms.c
> Hi Andrew,
> 
> Since I'll have to send next revision(v6, to modify patch 1/3) and 
> you've commented v4, would you also comment or Ack the 3/3 of this one? 
> This should save one revision for me to make any necessary change.

It looks OK to my inexpert eye.  It would be better if Maxim and/or Alex
could review the code.  If that doesn't happen then I guess the best we
can do is to go ahead and merge it.  Have you worked out via which tree
the patches will be merged?

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

VERY URGENT

[Andrew Smith]

Dr.Richard Anderson,
No 15 Alfateh St Damascus Syria

Hello Friend,

My name is Dr.Richard Anderson, a citizen of France a doctor
and personal adviser to the Syrian deputy defense minister,
late Assef Shawkat; who was killed on the 18th of July 2012,
we are all moving our money out of Syria I need your assistance
to receive the sum of $22,500.000.00 Dollars for further investment
under your care and management,
I will give you 30% of the total money for your assistance.
Contact me with this email address below for more details.
E-mail: richanderson...@gmail.com
Please reply urgent
Thanks
Dr.Richard Anderson

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

Fwd: Re: wilc1000 staging driver initialization failure

[Andrew Boyce]

Sorry for the mass forward, I was instructed to email more than just the 
linux-wireless mailing list.


Any help would be appreciated.

Thanks!
 -Andrew


 Forwarded Message 
Subject: Re: wilc1000 staging driver initialization failure
Date: Thu, 4 Feb 2016 13:26:04 -0500
From: Andrew Boyce 
To: linux-wireless-u79uwxl29ty76z2rm5m...@public.gmane.org
Newsgroups: gmane.linux.kernel.wireless.general
References: 

I've enabled more debugging.


The core of the issue seems to be that wilc_lock_timeout() is for some
reason failing to down_timeout(). I do not currently understand enough
of the internals to understand why.



[ 6852.53] wilc1000: module is from the staging directory, the
quality is unknown, you have been warned.
[ 6852.57] wilc1000_sdio: module is from the staging directory, the
quality is unknown, you have been warned.
[ 6852.58] wilc1000_sdio mmc0:0001:1: Initializing netdev
[ 6852.58] DBG [wilc_create_wiphy: 2783]Registering wifi device
[ 6852.58] DBG [WILC_WFI_CfgAlloc: 2747]Allocating wireless device
[ 6852.58] INFO [wilc_create_wiphy]Max number of PMKIDs = 16
[ 6852.58] INFO [wilc_create_wiphy]Max scan ids = 10,Max scan IE len
= 1000,Signal Type = 1,Interface Modes = 844,Interface Type = 2
[ 6852.58] DBG [wilc_create_wiphy: 2822]Successful Registering
[ 6852.58] DBG [wilc_create_wiphy: 2783]Registering wifi device
[ 6852.58] DBG [WILC_WFI_CfgAlloc: 2747]Allocating wireless device
[ 6852.58] INFO [wilc_create_wiphy]Max number of PMKIDs = 16
[ 6852.58] INFO [wilc_create_wiphy]Max scan ids = 10,Max scan IE len
= 1000,Signal Type = 1,Interface Modes = 844,Interface Type = 2
[ 6852.58] DBG [wilc_create_wiphy: 2822]Successful Registering
[ 6852.58] wilc1000_sdio mmc0:0001:1: Driver Initializing success
[ 6856.90] DBG [wilc_mac_open: 1020]MAC OPEN[cd8c9000]
[ 6856.90] DBG [wilc_init_host_int: 2835]Host[cd8c9000][cd97cd00]
[ 6856.90] DBG [wilc_init: 3790]Initializing host interface for client 1
[ 6856.90] DBG [wilc_init: 3810]Global handle pointer value=cee0f400
[ 6856.90] DBG [wilc_init: 3824]INIT: CLIENT COUNT 0
[ 6856.90] INFO [wilc_init]Initialization values, Site survey value: 2
 Scan source: 0
 Active scan time: 10
 Passive scan time: 1200
Current tx Rate = 0
[ 6856.90] DBG [wilc_mac_open: 1029]*** re-init ***
[ 6856.90] DBG [wlan_init_locks: 809]Initializing Locks ...
[ 6856.90] DBG [wilc_wlan_init: 1593]Initializing WILC_Wlan ...
[ 6856.90] ERR [wilc1000_wlan_init: 900]Initializing WILC_Wlan FAILED
[ 6856.90] ERR [wilc1000_wlan_init: 900]Initializing WILC_Wlan FAILED
[ 6856.90] DBG [wlan_deinit_locks: 835]De-Initializing Locks
[ 6856.90] ERR [wilc1000_wlan_init: 983]WLAN Iinitialization FAILED
[ 6856.90] ERR [wilc1000_wlan_init: 983]WLAN Iinitialization FAILED
[ 6856.90] ERR [wilc_mac_open: 1032]Failed to initialize wilc1000
[ 6856.90] ERR [wilc_mac_open: 1032]Failed to initialize wilc1000
[ 6856.90] DBG [wilc_deinit: 3896]De-initializing host interface for
client 1
[ 6856.90] DBG [wilc_deinit: 3905]>> Connect timer is active
[ 6856.90] DBG [wilc_send_config_pkt: 619]Sending config SET PACKET
WID:2085
[ 6856.90] Sending config SET PACKET WID:2085
[ 6856.90] Sending config SET PACKET val:
[ 6856.90] DBG [wilc_wlan_cfg_set: 1443][WILC]PACKET Commit with
sequence number 0
[ 6856.90] [WILC]PACKET Commit with sequence number 0
[ 6856.90] DBG [wilc_wlan_cfg_set: 1446]Processing cfg_set()
[ 6856.90] Processing cfg_set()
[ 6856.90] DBG [wilc_wlan_txq_add_cfg_pkt: 368]Adding config packet ...
[ 6856.90] DBG [wilc_wlan_txq_add_cfg_pkt: 389]Adding the config
packet at the Queue tail
[ 6856.90] DBG [wilc_lock_timeout: 257]Locking cd9eb82c
[ 6856.90] Locking cd9eb82c
[ 6856.90] timeout 2000
[ 6856.90] Failed, mutex is NULL
[ 6856.90] DBG [wilc_wlan_txq_add_to_head: 150]Number of entries in
TxQ = 1
[ 6856.90] DBG [wilc_wlan_txq_add_to_head: 155]Wake up the txq_handler
[ 6856.90] DBG [wilc_lock_timeout: 257]Locking cd9eb850
[ 6856.90] Locking cd9eb850
[ 6856.90] timeout 2000
[ 6858.90] Failed, mutex is NULL
[ 6858.90] DBG [hostIFthread: 2793]THREAD: Exiting HostIfThread
[ 6858.90] DBG [hostIFthread: 2989]Releasing thread exit semaphore
[ 6858.90] INFO [clear_shadow_scan]destroy aging timer
[ 6858.90] DBG [wilc_deinit_host_int: 2878]destroy during ip


Any help would be appreciated.

Thanks!
 -Andrew



On 2/3/16 6:09 PM, Andrew Boyce wrote:

I've been working to build and test the staging wilc1000-sdio driver and
have run into a problem.

I've built a kernel from Greg KH's staging-testing branch and loaded it
onto our device. When I try to bring the interface up, I get the following:

root@test:~# ifconfig wlan0 up
kernel: [ 3510.40] DBG [wilc_mac_open: 1014]MAC OPEN[cef2]
kernel: [ 3510.40] DBG [wilc_init_host_int:
2835]Host[

[PATCH] rtl8188eu: fix signal strength indication

[Andrew Bradford]

RTL8188E can only have a maximum of 2 chains so match that in the actual
phy_rx_agc_info structure within phy_status_rpt.  This will cause the
other data received from the PHY, such as signal strength indication of
beacons, to properly align and allow extraction and use within the
signal strength record-keeping mechanisms.

Signed-off-by: Andrew Bradford 
---
 drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h | 2 +-
 drivers/staging/rtl8188eu/include/odm_HWConfig.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h 
b/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h
index e058162..b8833fa 100644
--- a/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h
+++ b/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h
@@ -75,7 +75,7 @@ enum rf_radio_path {
 
 #define MAX_PG_GROUP 13
 
-#defineRF_PATH_MAX 3
+#defineRF_PATH_MAX 2
 #defineMAX_RF_PATH RF_PATH_MAX
 #defineMAX_TX_COUNT4 /* path numbers */
 
diff --git a/drivers/staging/rtl8188eu/include/odm_HWConfig.h 
b/drivers/staging/rtl8188eu/include/odm_HWConfig.h
index 62a0049..ef792bf 100644
--- a/drivers/staging/rtl8188eu/include/odm_HWConfig.h
+++ b/drivers/staging/rtl8188eu/include/odm_HWConfig.h
@@ -69,7 +69,7 @@ struct phy_rx_agc_info {
 };
 
 struct phy_status_rpt {
-   struct phy_rx_agc_info path_agc[3];
+   struct phy_rx_agc_info path_agc[RF_PATH_MAX];
u8  ch_corr[2];
u8  cck_sig_qual_ofdm_pwdb_all;
u8  cck_agc_rpt_ofdm_cfosho_a;
-- 
2.7.0

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

Re: [PATCH] rtl8188eu: fix signal strength indication

[Andrew Bradford]

Hi Larry,

On 02/13 13:50, Larry Finger wrote:
> On 02/11/2016 07:38 PM, Andrew Bradford wrote:
> >RTL8188E can only have a maximum of 2 chains so match that in the actual
> >phy_rx_agc_info structure within phy_status_rpt.  This will cause the
> >other data received from the PHY, such as signal strength indication of
> >beacons, to properly align and allow extraction and use within the
> >signal strength record-keeping mechanisms.
> >
> >Signed-off-by: Andrew Bradford 
> 
> Good catch. I only have two comments/suggestions. The first is minor - when
> sending patches for drivers in staging, it is customary to use "staging:" as
> the beginning of the subject. Accordingly, yours should be "staging:
> rtl8188eu: fix...".

OK, thanks, I'll be sure any future patches I send for the staging tree
comply.

> My second point is that with RF_PATH_MAX reduced to 2, there are parts of
> the code that are flagged with array overrun errors by Smatch. None of these
> statements will ever be executed, thus there are no run-time errors. I could
> fix these, but as such a patch would conflict with this one, I suggest you
> add a second patch. The file with needed changes is
> drivers/staging/rtl8188eu/hal/phy.c in the code that starts at line 183. The
> else if tests for TxCount == RF_PATH_C and TxCount == RF_PATH_D should be
> removed in their entirety.

OK, I will work on a patch to make the RF_PATH_C and RF_PATH_D changes.

Thanks for taking the time to review my patch! :)
-Andrew

> There are other changes that should be made, but as they do not conflict, I
> will do that myself.
> 
> Thanks,
> 
> Larry
> 
> >---
> >  drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h | 2 +-
> >  drivers/staging/rtl8188eu/include/odm_HWConfig.h   | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h 
> >b/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h
> >index e058162..b8833fa 100644
> >--- a/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h
> >+++ b/drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h
> >@@ -75,7 +75,7 @@ enum rf_radio_path {
> >
> >  #define MAX_PG_GROUP 13
> >
> >-#define RF_PATH_MAX 3
> >+#define RF_PATH_MAX 2
> >  #defineMAX_RF_PATH RF_PATH_MAX
> >  #defineMAX_TX_COUNT4 /* path numbers */
> >
> >diff --git a/drivers/staging/rtl8188eu/include/odm_HWConfig.h 
> >b/drivers/staging/rtl8188eu/include/odm_HWConfig.h
> >index 62a0049..ef792bf 100644
> >--- a/drivers/staging/rtl8188eu/include/odm_HWConfig.h
> >+++ b/drivers/staging/rtl8188eu/include/odm_HWConfig.h
> >@@ -69,7 +69,7 @@ struct phy_rx_agc_info {
> >  };
> >
> >  struct phy_status_rpt {
> >-struct phy_rx_agc_info path_agc[3];
> >+struct phy_rx_agc_info path_agc[RF_PATH_MAX];
> > u8  ch_corr[2];
> > u8  cck_sig_qual_ofdm_pwdb_all;
> > u8  cck_agc_rpt_ofdm_cfosho_a;
> >
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rtl8188eu: Remove RF_PATH_C & RF_PATH_D

[Andrew Bradford]

RTL8188EE has a maximum of 2 RF paths (chains) so paths C and D are not
needed to support this part.

Signed-off-by: Andrew Bradford 
---
 drivers/staging/rtl8188eu/hal/bb_cfg.c | 26 --
 drivers/staging/rtl8188eu/hal/phy.c| 26 --
 drivers/staging/rtl8188eu/include/Hal8188EPhyCfg.h |  2 --
 3 files changed, 54 deletions(-)

diff --git a/drivers/staging/rtl8188eu/hal/bb_cfg.c 
b/drivers/staging/rtl8188eu/hal/bb_cfg.c
index f58a822..c2ad6a3 100644
--- a/drivers/staging/rtl8188eu/hal/bb_cfg.c
+++ b/drivers/staging/rtl8188eu/hal/bb_cfg.c
@@ -598,18 +598,12 @@ static void 
rtl88e_phy_init_bb_rf_register_definition(struct adapter *adapter)
 
reg[RF_PATH_A] = &hal_data->PHYRegDef[RF_PATH_A];
reg[RF_PATH_B] = &hal_data->PHYRegDef[RF_PATH_B];
-   reg[RF_PATH_C] = &hal_data->PHYRegDef[RF_PATH_C];
-   reg[RF_PATH_D] = &hal_data->PHYRegDef[RF_PATH_D];
 
reg[RF_PATH_A]->rfintfs = rFPGA0_XAB_RFInterfaceSW;
reg[RF_PATH_B]->rfintfs = rFPGA0_XAB_RFInterfaceSW;
-   reg[RF_PATH_C]->rfintfs = rFPGA0_XCD_RFInterfaceSW;
-   reg[RF_PATH_D]->rfintfs = rFPGA0_XCD_RFInterfaceSW;
 
reg[RF_PATH_A]->rfintfi = rFPGA0_XAB_RFInterfaceRB;
reg[RF_PATH_B]->rfintfi = rFPGA0_XAB_RFInterfaceRB;
-   reg[RF_PATH_C]->rfintfi = rFPGA0_XCD_RFInterfaceRB;
-   reg[RF_PATH_D]->rfintfi = rFPGA0_XCD_RFInterfaceRB;
 
reg[RF_PATH_A]->rfintfo = rFPGA0_XA_RFInterfaceOE;
reg[RF_PATH_B]->rfintfo = rFPGA0_XB_RFInterfaceOE;
@@ -622,13 +616,9 @@ static void 
rtl88e_phy_init_bb_rf_register_definition(struct adapter *adapter)
 
reg[RF_PATH_A]->rfLSSI_Select = rFPGA0_XAB_RFParameter;
reg[RF_PATH_B]->rfLSSI_Select = rFPGA0_XAB_RFParameter;
-   reg[RF_PATH_C]->rfLSSI_Select = rFPGA0_XCD_RFParameter;
-   reg[RF_PATH_D]->rfLSSI_Select = rFPGA0_XCD_RFParameter;
 
reg[RF_PATH_A]->rfTxGainStage = rFPGA0_TxGainStage;
reg[RF_PATH_B]->rfTxGainStage = rFPGA0_TxGainStage;
-   reg[RF_PATH_C]->rfTxGainStage = rFPGA0_TxGainStage;
-   reg[RF_PATH_D]->rfTxGainStage = rFPGA0_TxGainStage;
 
reg[RF_PATH_A]->rfHSSIPara1 = rFPGA0_XA_HSSIParameter1;
reg[RF_PATH_B]->rfHSSIPara1 = rFPGA0_XB_HSSIParameter1;
@@ -638,43 +628,27 @@ static void 
rtl88e_phy_init_bb_rf_register_definition(struct adapter *adapter)
 
reg[RF_PATH_A]->rfSwitchControl = rFPGA0_XAB_SwitchControl;
reg[RF_PATH_B]->rfSwitchControl = rFPGA0_XAB_SwitchControl;
-   reg[RF_PATH_C]->rfSwitchControl = rFPGA0_XCD_SwitchControl;
-   reg[RF_PATH_D]->rfSwitchControl = rFPGA0_XCD_SwitchControl;
 
reg[RF_PATH_A]->rfAGCControl1 = rOFDM0_XAAGCCore1;
reg[RF_PATH_B]->rfAGCControl1 = rOFDM0_XBAGCCore1;
-   reg[RF_PATH_C]->rfAGCControl1 = rOFDM0_XCAGCCore1;
-   reg[RF_PATH_D]->rfAGCControl1 = rOFDM0_XDAGCCore1;
 
reg[RF_PATH_A]->rfAGCControl2 = rOFDM0_XAAGCCore2;
reg[RF_PATH_B]->rfAGCControl2 = rOFDM0_XBAGCCore2;
-   reg[RF_PATH_C]->rfAGCControl2 = rOFDM0_XCAGCCore2;
-   reg[RF_PATH_D]->rfAGCControl2 = rOFDM0_XDAGCCore2;
 
reg[RF_PATH_A]->rfRxIQImbalance = rOFDM0_XARxIQImbalance;
reg[RF_PATH_B]->rfRxIQImbalance = rOFDM0_XBRxIQImbalance;
-   reg[RF_PATH_C]->rfRxIQImbalance = rOFDM0_XCRxIQImbalance;
-   reg[RF_PATH_D]->rfRxIQImbalance = rOFDM0_XDRxIQImbalance;
 
reg[RF_PATH_A]->rfRxAFE = rOFDM0_XARxAFE;
reg[RF_PATH_B]->rfRxAFE = rOFDM0_XBRxAFE;
-   reg[RF_PATH_C]->rfRxAFE = rOFDM0_XCRxAFE;
-   reg[RF_PATH_D]->rfRxAFE = rOFDM0_XDRxAFE;
 
reg[RF_PATH_A]->rfTxIQImbalance = rOFDM0_XATxIQImbalance;
reg[RF_PATH_B]->rfTxIQImbalance = rOFDM0_XBTxIQImbalance;
-   reg[RF_PATH_C]->rfTxIQImbalance = rOFDM0_XCTxIQImbalance;
-   reg[RF_PATH_D]->rfTxIQImbalance = rOFDM0_XDTxIQImbalance;
 
reg[RF_PATH_A]->rfTxAFE = rOFDM0_XATxAFE;
reg[RF_PATH_B]->rfTxAFE = rOFDM0_XBTxAFE;
-   reg[RF_PATH_C]->rfTxAFE = rOFDM0_XCTxAFE;
-   reg[RF_PATH_D]->rfTxAFE = rOFDM0_XDTxAFE;
 
reg[RF_PATH_A]->rfLSSIReadBack = rFPGA0_XA_LSSIReadBack;
reg[RF_PATH_B]->rfLSSIReadBack = rFPGA0_XB_LSSIReadBack;
-   reg[RF_PATH_C]->rfLSSIReadBack = rFPGA0_XC_LSSIReadBack;
-   reg[RF_PATH_D]->rfLSSIReadBack = rFPGA0_XD_LSSIReadBack;
 
reg[RF_PATH_A]->rfLSSIReadBackPi = TransceiverA_HSPI_Readback;
reg[RF_PATH_B]->rfLSSIReadBackPi = TransceiverB_HSPI_Readback;
diff --git a/drivers/staging/rtl8188eu/hal/phy.c 
b/drivers/staging/rtl8188eu/hal/phy.c
index d3e8a8e..ae42b44 100644
--- a/drivers/staging/rtl8188eu/hal/phy.c
+++ b/drivers/staging/rtl8188eu/hal/phy.c
@@ -180,32 +180,6 @@ static void get_tx_power_index(struct adapter *adapt, u8 
channel, u8 *cck_pwr,
hal_data

Re: [PATCH 1/3] driver core: export lock_device_hotplug/unlock_device_hotplug

[Andrew Morton]

On Wed, 11 Feb 2015 16:44:20 +0100 Vitaly Kuznetsov  wrote:

> add_memory() is supposed to be run with device_hotplug_lock grabbed, otherwise
> it can race with e.g. device_online(). Allow external modules (hv_balloon for
> now) to lock device hotplug.
> 
> ...
>
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -55,11 +55,13 @@ void lock_device_hotplug(void)
>  {
>   mutex_lock(&device_hotplug_lock);
>  }
> +EXPORT_SYMBOL_GPL(lock_device_hotplug);
>  
>  void unlock_device_hotplug(void)
>  {
>   mutex_unlock(&device_hotplug_lock);
>  }
> +EXPORT_SYMBOL_GPL(unlock_device_hotplug);
>  
>  int lock_device_hotplug_sysfs(void)
>  {

It's kinda crazy that lock_device_hotplug_sysfs() didn't get any
documentation.  I suggest adding this while you're in there:


--- a/drivers/base/core.c~a
+++ a/drivers/base/core.c
@@ -61,6 +61,9 @@ void unlock_device_hotplug(void)
mutex_unlock(&device_hotplug_lock);
 }
 
+/*
+ * "git show 5e33bc4165f3ed" for details
+ */
 int lock_device_hotplug_sysfs(void)
 {
if (mutex_trylock(&device_hotplug_lock))

which is a bit lazy but whatev.

I'll assume that Greg (or Rafael?) will be processing this patchset.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND 0/3] memory_hotplug: hyperv: fix deadlock between memory adding and onlining

[Andrew Morton]

On Thu, 12 Feb 2015 23:43:17 +0100 "Rafael J. Wysocki"  
wrote:

> On Thursday, February 12, 2015 10:10:30 PM KY Srinivasan wrote:
> 
> [cut]

yay!

> > > > > >
> > > > > > This issue was first discovered by Andy Whitcroft:
> > > > > > https://lkml.org/lkml/2014/3/14/451
> > > > > > I had sent patches based on Andy's analysis that did not affect
> > > > > > the users of the kernel hot-add memory APIs:
> > > > > > https://lkml.org/lkml/2014/12/2/662
> > > > > >
> > > > > > This patch puts the burden where it needs to be and can address
> > > > > > the issue
> > > > > for all clients.
> > > > >
> > > > > That seems to mean that this series is not needed.  Is that correct?
> > > >
> > > > This patch was never committed upstream and so the issue still is there.
> > > 
> > > Well, I'm not sure what to do now to be honest.
> > > 
> > > Is this series regarded as the right way to address the problem that
> > > everybody is comfortable with?  Or is it still under discussion?
> > 
> > We need to solve this problem and that is not under discussion. I also 
> > believe this problem
> > needs to be solved in a way that addresses the problem where it belongs - 
> > not in the users of
> > the hot_add API. Both my solution and the one proposed by David 
> > https://lkml.org/lkml/2015/2/12/57
> > address this issue. You can select either patch and check it in. I just 
> > want the issue addressed and I am not
> > married to the solution I proposed.
> 
> OK, thanks!
> 
> So having looked at both your patch and the David's one I think that
> the Andrew's tree is appropriate for any of them.
> 
> Andrew?

OK, I'll wake up and take a look.  Hopefully as 3.21 material but I
need to to back and reread everything.  Is it more urgent than that?

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

[PATCH] Implement Hyper-V netvsc_get_channels() ethool op

[Andrew Schwartzmeyer]

This adds support for reporting the combined channels count of the
hv_netvsc driver via 'ethtool --show-channels'.

Signed-off-by: Andrew Schwartzmeyer 
---
 drivers/net/hyperv/netvsc_drv.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 15d82eda0baf..7106e3f88bde 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -687,6 +687,27 @@ static void netvsc_get_drvinfo(struct net_device *net,
strlcpy(info->fw_version, "N/A", sizeof(info->fw_version));
 }
 
+static int netvsc_get_channels_combined_count(struct net_device *net)
+{
+   struct net_device_context *net_device_ctx = netdev_priv(net);
+   struct hv_device *dev = net_device_ctx->device_ctx;
+   struct netvsc_device *nvdev = hv_get_drvdata(dev);
+
+   if (!nvdev)
+   return 0;
+   return nvdev->num_chn;
+}
+
+static void netvsc_get_channels(struct net_device *net,
+   struct ethtool_channels *channel)
+{
+   int ret;
+
+   ret = netvsc_get_channels_combined_count(net);
+   if (ret > 0)
+   channel->combined_count = ret;
+}
+
 static int netvsc_change_mtu(struct net_device *ndev, int mtu)
 {
struct net_device_context *ndevctx = netdev_priv(ndev);
@@ -760,6 +781,7 @@ static void netvsc_poll_controller(struct net_device *net)
 static const struct ethtool_ops ethtool_ops = {
.get_drvinfo= netvsc_get_drvinfo,
.get_link   = ethtool_op_get_link,
+   .get_channels   = netvsc_get_channels,
 };
 
 static const struct net_device_ops device_ops = {
-- 
2.3.0

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

[PATCH] Implement Hyper-V netvsc_get_channels() ethool op

[Andrew Schwartzmeyer]

This adds support for reporting the combined channels count of the
hv_netvsc driver via 'ethtool --show-channels'.

This required adding 'max_chn' to 'struct netvsc_device', and assigning
it 'rsscap.num_recv_que' in 'rndis_filter_device_add'. Now we can access
the combined maximum channel count via 'struct netvsc_device' in the
ethtool callback.

Signed-off-by: Andrew Schwartzmeyer 
---
 drivers/net/hyperv/hyperv_net.h   |  1 +
 drivers/net/hyperv/netvsc_drv.c   | 14 ++
 drivers/net/hyperv/rndis_filter.c |  5 -
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 384ca4f4de4a..371f2a6c0715 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -635,6 +635,7 @@ struct netvsc_device {
struct vmbus_channel *chn_table[NR_CPUS];
u32 send_table[VRSS_SEND_TAB_SIZE];
u32 num_chn;
+   u32 max_chn;
atomic_t queue_sends[NR_CPUS];
 
/* Holds rndis device info */
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 15d82eda0baf..a06bd6614007 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -687,6 +687,19 @@ static void netvsc_get_drvinfo(struct net_device *net,
strlcpy(info->fw_version, "N/A", sizeof(info->fw_version));
 }
 
+static void netvsc_get_channels(struct net_device *net,
+   struct ethtool_channels *channel)
+{
+   struct net_device_context *net_device_ctx = netdev_priv(net);
+   struct hv_device *dev = net_device_ctx->device_ctx;
+   struct netvsc_device *nvdev = hv_get_drvdata(dev);
+
+   if (nvdev) {
+   channel->max_combined   = nvdev->max_chn;
+   channel->combined_count = nvdev->num_chn;
+   }
+}
+
 static int netvsc_change_mtu(struct net_device *ndev, int mtu)
 {
struct net_device_context *ndevctx = netdev_priv(ndev);
@@ -760,6 +773,7 @@ static void netvsc_poll_controller(struct net_device *net)
 static const struct ethtool_ops ethtool_ops = {
.get_drvinfo= netvsc_get_drvinfo,
.get_link   = ethtool_op_get_link,
+   .get_channels   = netvsc_get_channels,
 };
 
 static const struct net_device_ops device_ops = {
diff --git a/drivers/net/hyperv/rndis_filter.c 
b/drivers/net/hyperv/rndis_filter.c
index 7816d98bdddc..015f9b9b5093 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1094,6 +1094,7 @@ int rndis_filter_device_add(struct hv_device *dev,
if (ret || rsscap.num_recv_que < 2)
goto out;
 
+   net_device->max_chn = rsscap.num_recv_que;
net_device->num_chn = (num_online_cpus() < rsscap.num_recv_que) ?
   num_online_cpus() : rsscap.num_recv_que;
if (net_device->num_chn == 1)
@@ -1140,8 +1141,10 @@ int rndis_filter_device_add(struct hv_device *dev,
ret = rndis_filter_set_rss_param(rndis_device, net_device->num_chn);
 
 out:
-   if (ret)
+   if (ret) {
+   net_device->max_chn = 1;
net_device->num_chn = 1;
+   }
return 0; /* return 0 because primary channel can be used alone */
 
 err_dev_remv:
-- 
2.3.0

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

[PATCH net-next] hyperv: Implement netvsc_get_channels() ethool op

[Andrew Schwartzmeyer]

This adds support for reporting the actual and maximum combined channels
count of the hv_netvsc driver via 'ethtool --show-channels'.

This required adding 'max_chn' to 'struct netvsc_device', and assigning
it 'rsscap.num_recv_que' in 'rndis_filter_device_add'. Now we can access
the combined maximum channel count via 'struct netvsc_device' in the
ethtool callback.

Signed-off-by: Andrew Schwartzmeyer 
---
 drivers/net/hyperv/hyperv_net.h   |  1 +
 drivers/net/hyperv/netvsc_drv.c   | 14 ++
 drivers/net/hyperv/rndis_filter.c |  6 +-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 384ca4f4de4a..4815843a6019 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -634,6 +634,7 @@ struct netvsc_device {
 
struct vmbus_channel *chn_table[NR_CPUS];
u32 send_table[VRSS_SEND_TAB_SIZE];
+   u32 max_chn;
u32 num_chn;
atomic_t queue_sends[NR_CPUS];
 
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 15d82eda0baf..a06bd6614007 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -687,6 +687,19 @@ static void netvsc_get_drvinfo(struct net_device *net,
strlcpy(info->fw_version, "N/A", sizeof(info->fw_version));
 }
 
+static void netvsc_get_channels(struct net_device *net,
+   struct ethtool_channels *channel)
+{
+   struct net_device_context *net_device_ctx = netdev_priv(net);
+   struct hv_device *dev = net_device_ctx->device_ctx;
+   struct netvsc_device *nvdev = hv_get_drvdata(dev);
+
+   if (nvdev) {
+   channel->max_combined   = nvdev->max_chn;
+   channel->combined_count = nvdev->num_chn;
+   }
+}
+
 static int netvsc_change_mtu(struct net_device *ndev, int mtu)
 {
struct net_device_context *ndevctx = netdev_priv(ndev);
@@ -760,6 +773,7 @@ static void netvsc_poll_controller(struct net_device *net)
 static const struct ethtool_ops ethtool_ops = {
.get_drvinfo= netvsc_get_drvinfo,
.get_link   = ethtool_op_get_link,
+   .get_channels   = netvsc_get_channels,
 };
 
 static const struct net_device_ops device_ops = {
diff --git a/drivers/net/hyperv/rndis_filter.c 
b/drivers/net/hyperv/rndis_filter.c
index 7816d98bdddc..ca81de04bc76 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1027,6 +1027,7 @@ int rndis_filter_device_add(struct hv_device *dev,
 
/* Initialize the rndis device */
net_device = hv_get_drvdata(dev);
+   net_device->max_chn = 1;
net_device->num_chn = 1;
 
net_device->extension = rndis_device;
@@ -1094,6 +1095,7 @@ int rndis_filter_device_add(struct hv_device *dev,
if (ret || rsscap.num_recv_que < 2)
goto out;
 
+   net_device->max_chn = rsscap.num_recv_que;
net_device->num_chn = (num_online_cpus() < rsscap.num_recv_que) ?
   num_online_cpus() : rsscap.num_recv_que;
if (net_device->num_chn == 1)
@@ -1140,8 +1142,10 @@ int rndis_filter_device_add(struct hv_device *dev,
ret = rndis_filter_set_rss_param(rndis_device, net_device->num_chn);
 
 out:
-   if (ret)
+   if (ret) {
+   net_device->max_chn = 1;
net_device->num_chn = 1;
+   }
return 0; /* return 0 because primary channel can be used alone */
 
 err_dev_remv:
-- 
2.3.0

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

drivers: staging: vme: Fixed some code style warnings

[Andrew Kanner]

Signed-off-by: Andrew Kanner 
---
 drivers/staging/vme/devices/vme_pio2_core.c | 12 ++--
 drivers/staging/vme/devices/vme_user.c  |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_pio2_core.c 
b/drivers/staging/vme/devices/vme_pio2_core.c
index 28a4568..8e66a52 100644
--- a/drivers/staging/vme/devices/vme_pio2_core.c
+++ b/drivers/staging/vme/devices/vme_pio2_core.c
@@ -466,23 +466,23 @@ static void __exit pio2_exit(void)
 
 /* These are required for each board */
 MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the board is connected");
-module_param_array(bus, int, &bus_num, S_IRUGO);
+module_param_array(bus, int, &bus_num, 0444);
 
 MODULE_PARM_DESC(base, "Base VME address for PIO2 Registers");
-module_param_array(base, long, &base_num, S_IRUGO);
+module_param_array(base, long, &base_num, 0444);
 
 MODULE_PARM_DESC(vector, "VME IRQ Vector (Lower 4 bits masked)");
-module_param_array(vector, int, &vector_num, S_IRUGO);
+module_param_array(vector, int, &vector_num, 0444);
 
 MODULE_PARM_DESC(level, "VME IRQ Level");
-module_param_array(level, int, &level_num, S_IRUGO);
+module_param_array(level, int, &level_num, 0444);
 
 MODULE_PARM_DESC(variant, "Last 4 characters of PIO2 board variant");
-module_param_array(variant, charp, &variant_num, S_IRUGO);
+module_param_array(variant, charp, &variant_num, 0444);
 
 /* This is for debugging */
 MODULE_PARM_DESC(loopback, "Enable loopback mode on all cards");
-module_param(loopback, bool, S_IRUGO);
+module_param(loopback, bool, 0444);
 
 MODULE_DESCRIPTION("GE PIO2 6U VME I/O Driver");
 MODULE_AUTHOR("Martyn Welch http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: drivers: staging: vme: Fixed some code style warnings

[Andrew Kanner]

‎Thanks, I understood my fault, but haven't done this changes yet. I can't 
understand if I should reply to original message with v2 patch or send a new 
email with it?


  Исходное сообщение  
От: Markus Böhme
Отправлено: среда, 14 сентября 2016 г., 15:56
Кому: Andrew Kanner; gre...@linuxfoundation.org
Копия: de...@driverdev.osuosl.org; manohar.va...@gmail.com; 
egor.ulieis...@gmail.com; linux-ker...@vger.kernel.org
Тема: Re: drivers: staging: vme: Fixed some code style warnings

On 09/13/2016 12:31 AM, Andrew Kanner wrote:
> Signed-off-by: Andrew Kanner 
> ---
> drivers/staging/vme/devices/vme_pio2_core.c | 12 ++--
> drivers/staging/vme/devices/vme_user.c | 2 +-
> 2 files changed, 7 insertions(+), 7 deletions(-)
> (snip)

Hello Andrew,

please be more specific in your subject line, e.g.
"drivers: staging: vme: Convert to octal notation for permission bits".

Also don't forget to add a commit message to your patch with a short
description what you are fixing and why. In your case it would be good
to mention that you are fixing a checkpatch warning, and to include the
warning message in your description. Then resend as V2.

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

[PATCH v2] drivers: staging: vme: convert to octal notation for permission bits

[Andrew Kanner]

Ran checkpatch.pl -f vme_pio2_core.c
Fixed: WARNING: Symbolic permissions are not preferred. Consider using
octal permissions (0444)

Signed-off-by: Andrew Kanner 
---
 drivers/staging/vme/devices/vme_pio2_core.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_pio2_core.c 
b/drivers/staging/vme/devices/vme_pio2_core.c
index 28a4568..8e66a52 100644
--- a/drivers/staging/vme/devices/vme_pio2_core.c
+++ b/drivers/staging/vme/devices/vme_pio2_core.c
@@ -466,23 +466,23 @@ static void __exit pio2_exit(void)
 
 /* These are required for each board */
 MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the board is connected");
-module_param_array(bus, int, &bus_num, S_IRUGO);
+module_param_array(bus, int, &bus_num, 0444);
 
 MODULE_PARM_DESC(base, "Base VME address for PIO2 Registers");
-module_param_array(base, long, &base_num, S_IRUGO);
+module_param_array(base, long, &base_num, 0444);
 
 MODULE_PARM_DESC(vector, "VME IRQ Vector (Lower 4 bits masked)");
-module_param_array(vector, int, &vector_num, S_IRUGO);
+module_param_array(vector, int, &vector_num, 0444);
 
 MODULE_PARM_DESC(level, "VME IRQ Level");
-module_param_array(level, int, &level_num, S_IRUGO);
+module_param_array(level, int, &level_num, 0444);
 
 MODULE_PARM_DESC(variant, "Last 4 characters of PIO2 board variant");
-module_param_array(variant, charp, &variant_num, S_IRUGO);
+module_param_array(variant, charp, &variant_num, 0444);
 
 /* This is for debugging */
 MODULE_PARM_DESC(loopback, "Enable loopback mode on all cards");
-module_param(loopback, bool, S_IRUGO);
+module_param(loopback, bool, 0444);
 
 MODULE_DESCRIPTION("GE PIO2 6U VME I/O Driver");
 MODULE_AUTHOR("Martyn Welch http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC v3 3/3] phy,leds: add support for led triggers on phy link state change

[Andrew Lunn]

> Andrew, are you happy with this implementation?

Sorry, been to busy with other things to follow the discussion.

What would be nice to see is a comment about how the link to LEDs in
the PHYs is made. Often there is a couple of LEDs in the RJ45 socket
driven by the PHY. They can show link, speed, packet Rx/Tx, etc. How
are these triggers related to these LEDs?

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

Re: [PATCH v4 3/3] net: phy: leds: add support for led triggers on phy link state change

[Andrew Lunn]

> Do you have suggestions on how to better handle the choice of the array size
> and the speeds?

phydev->supported lists the speeds this phy supports.

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

Re: [PATCH v5 4/4] net: phy: leds: add support for led triggers on phy link state change

[Andrew Lunn]

On Mon, Oct 17, 2016 at 10:49:55AM -0500, Zach Brown wrote:
> Create an option CONFIG_LED_TRIGGER_PHY (default n), which will create a
> set of led triggers for each instantiated PHY device. There is one LED
> trigger per link-speed, per-phy.
> The triggers are registered during phy_attach and unregistered during
> phy_detach.
> 
> This allows for a user to configure their system to allow a set of LEDs
> not controlled by the phy to represent link state changes on the phy.
> LEDS controlled by the phy are unaffected.
> 
> For example, we have a board where some of the leds in the
> RJ45 socket are controlled by the phy, but others are not. Using the
> triggers provided by this patch the leds not controlled by the phy can
> be configured to show the current speed of the ethernet connection. The
> leds controlled by the phy are unaffected.

Is there a clear path how we generalise this, so it could also be used
to control PHY LEDs?

The idea i had a while ago was that the PHY LEDS would also have a
list of triggers which mapped to what the PHY controller could do. So
to enable the PHY LED to show RxTx activity, you would enable the RxTx
trigger on the PHY LED.

This needs an extension to the PHY code, in that these triggers are
specific to the LED, not general across all LEDs. But this is not too
big an issue.

We could end up that if a PHY LED can be used as a generic LED, your
'manual' trigger could be used on it. But it could also support the
PHY driven 'automatic' link status indication trigger. Do we want two
triggers for the same or similar information?

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

[PATCH] staging: lustre: fixed shadowed variable in socklnd_cb.c

[Andrew Kanner]

Changed variable 'tx' name in local scope
Fixed: sparse warning:
socklnd_cb.c:2476:41: warning: symbol 'tx' shadows an earlier one
socklnd_cb.c:2435:25: originally declared here

Signed-off-by: Andrew Kanner 
---
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
index c1c6f60..03fe4e5 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
@@ -2473,11 +2473,11 @@ ksocknal_check_peer_timeouts(int idx)
 * holding only shared lock
 */
if (!list_empty(&peer->ksnp_tx_queue)) {
-   struct ksock_tx *tx = 
list_entry(peer->ksnp_tx_queue.next,
+   struct ksock_tx *_tx = 
list_entry(peer->ksnp_tx_queue.next,
struct ksock_tx, tx_list);
 
if (cfs_time_aftereq(cfs_time_current(),
-tx->tx_deadline)) {
+_tx->tx_deadline)) {
ksocknal_peer_addref(peer);
read_unlock(&ksocknal_data.ksnd_global_lock);
 
-- 
2.1.4

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

Re: [PATCH] staging: lustre: fixed shadowed variable in socklnd_cb.c

[Andrew Kanner]

2016-11-04 1:12 GMT+03:00 Dilger, Andreas :
> On Nov 3, 2016, at 15:54, Andrew Kanner  wrote:
>>
>> Changed variable 'tx' name in local scope
>> Fixed: sparse warning:
>> socklnd_cb.c:2476:41: warning: symbol 'tx' shadows an earlier one
>> socklnd_cb.c:2435:25: originally declared here
>
> Looking at this more closely (or from a greater distance, hard to say),
> the outer-scope "tx" is used only after this inner-scope "tx", so in
> fact there is no benefit to having the inner-scope declaration at all.
>
> Removing it may save a stack variable (depending on how the compiler
> optimizes), and shouldn't affect functionality.
>
> Cheers, Andreas

I see, I'll correct it and resend as PATCH v2. Thank you.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: lustre: fixed shadowed variable in socklnd_cb.c

[Andrew Kanner]

Removed redundant declaration of variable 'tx' in local scope
Fixed: sparse warning:
socklnd_cb.c:2476:41: warning: symbol 'tx' shadows an earlier one
socklnd_cb.c:2435:25: originally declared here

Signed-off-by: Andrew Kanner 
---
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
index c1c6f60..f31f4a1 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
@@ -2473,8 +2473,8 @@ ksocknal_check_peer_timeouts(int idx)
 * holding only shared lock
 */
if (!list_empty(&peer->ksnp_tx_queue)) {
-   struct ksock_tx *tx = 
list_entry(peer->ksnp_tx_queue.next,
-   struct ksock_tx, tx_list);
+   tx = list_entry(peer->ksnp_tx_queue.next,
+   struct ksock_tx, tx_list);
 
if (cfs_time_aftereq(cfs_time_current(),
 tx->tx_deadline)) {
-- 
2.1.4

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

Re: [net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

[Andrew Lunn]

> +static const char slic_stats_strings[][ETH_GSTRING_LEN] = {
> + "rx_packets ",
> + "rx_bytes   ",
> + "rx_multicasts  ",
> + "rx_errors  ",
> + "rx_buff_miss   ",
> + "rx_tp_csum ",
> + "rx_tp_oflow",
> + "rx_tp_hlen ",
> + "rx_ip_csum ",
> + "rx_ip_len  ",

Are there any other drivers which pad the statistics strings?

> +static void slic_set_link_autoneg(struct slic_device *sdev)
> +{
> + unsigned int subid = sdev->pdev->subsystem_device;
> + u32 val;
> +
> + if (sdev->is_fiber) {
> + /* We've got a fiber gigabit interface, and register 4 is
> +  * different in fiber mode than in copper mode.
> +  */
> + /* advertise FD only @1000 Mb */
> + val = MII_ADVERTISE << 16 | SLIC_PAR_ADV1000XFD |
> +   SLIC_PAR_ASYMPAUSE_FIBER;
> + /* enable PAUSE frames */
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + /* reset phy, enable auto-neg  */
> + val = MII_BMCR << 16 | SLIC_PCR_RESET | SLIC_PCR_AUTONEG |
> +   SLIC_PCR_AUTONEG_RST;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + } else {/* copper gigabit */
> + /* We've got a copper gigabit interface, and register 4 is
> +  * different in copper mode than in fiber mode.
> +  */
> + /* advertise 10/100 Mb modes   */
> + val = MII_ADVERTISE << 16 | SLIC_PAR_ADV100FD |
> +   SLIC_PAR_ADV100HD | SLIC_PAR_ADV10FD | SLIC_PAR_ADV10HD;
> + /* enable PAUSE frames  */
> + val |= SLIC_PAR_ASYMPAUSE;
> + /* required by the Cicada PHY  */
> + val |= SLIC_PAR_802_3;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> +
> + /* advertise FD only @1000 Mb  */
> + val = MII_CTRL1000 << 16 | SLIC_PGC_ADV1000FD;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> +
> + if (subid != PCI_SUBDEVICE_ID_ALACRITECH_CICADA) {
> +  /* if a Marvell PHY enable auto crossover */
> + val = SLIC_MIICR_REG_16 | SLIC_MRV_REG16_XOVERON;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> +
> + /* reset phy, enable auto-neg  */
> + val = MII_BMCR << 16 | SLIC_PCR_RESET |
> +   SLIC_PCR_AUTONEG | SLIC_PCR_AUTONEG_RST;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + } else {
> + /* enable and restart auto-neg (don't reset)  */
> + val = MII_BMCR << 16 | SLIC_PCR_AUTONEG |
> +   SLIC_PCR_AUTONEG_RST;
> + slic_write(sdev, SLIC_REG_WPHY, val);
> + }
> + }
> + sdev->autoneg = true;
> +}

Could this be pulled out into a standard PHY driver? All the SLIC
SLIC_PCR_ defines seems to be the same as those in mii.h. This could
be a standard PHY hidden behind a single register.

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

Re: [net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

[Andrew Lunn]

> The link state is retrieved by a command to the application processor that is 
> running 
> on the network card. Also the register to set the phy configuration is 
> write-only, so
> it is not even possible to do the usual mdio bit-banging in the Phy read() 
> and write()
> functions (however there seems to be another application processor command 
> reserved 
> for retrieving the PHY settings, but I have not tried it yet). 

>> +val = MII_BMCR << 16 | SLIC_PCR_AUTONEG |
>> + SLIC_PCR_AUTONEG_RST;
>> +slic_write(sdev, SLIC_REG_WPHY, val);

This actually looks a lot like an MDIO write operation. The upper 16
bits are the register, and the lower 16 bits are the data. What you
don't have is the address. But maybe it is limited to one address.

If the processor command reserved for read works in a similar way, you
have enough to do an MDIO bus.

> Please also note that I do not have any datasheets or other documentation for 
> the hardware, 
> all I have as a reference is the driver code in staging. So I do not know 
> which 
> PHYs are actually used (the comments in the code mention Marvell and Cicada 
> but this is
> not very specific).

If you can get the read working look at registers 2 and 3. Compare
what you get with the values at the end of marvell.c.

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

Re: [net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

[Andrew Lunn]

> >>>>> +   val = MII_BMCR << 16 | SLIC_PCR_AUTONEG |
> >>>>> +SLIC_PCR_AUTONEG_RST;
> >>>>> +   slic_write(sdev, SLIC_REG_WPHY, val);

> Thats essentially what I meant by setting a flag in the irq handler. The mdio
> function would have to check somehow if the irq has been fired (be it by means
> of a flag or a completion that is set by the irq handler and checked by the 
> mdio function). So I agree that it should work (if reading via the AP command
> is actually possible).

It seems odd you have a nice simple way to do writes, but reads are
very complex. There might be a simple read method hiding somewhere.

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

Re: [PATCH] staging: rtlwifi: Improve debugging by using debugfs

[Andrew Lunn]

On Thu, Aug 24, 2017 at 04:28:08PM -0500, Larry Finger wrote:
> The changes in this commit are also being sent to the main rtlwifi
> drivers in wireless-next; however, these changes will also be useful for
> any debugging of r8822be before it gets moved into the main tree.
> 
> Use debugfs to dump register and btcoex status, and also write registers
> and h2c.
> 
> We create topdir in /sys/kernel/debug/rtlwifi/, and use the MAC address
> as subdirectory with several entries to dump mac_reg, bb_reg, rf_reg etc.
> An example is
> /sys/kernel/debug/rtlwifi/00-11-22-33-44-55-66/mac_0
> 
> This change permits examination of device registers in a dynamic manner,
> a feature not available with the current debug mechanism.

Hi Larry

netdev frowns upon debugfs. You should try to keep this altogether,
making it easy to throw away before the driver is moved out of
staging.

You might want to look at ethtool -d. That will be accepted.

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

Re: [PATCH] staging: rtlwifi: Improve debugging by using debugfs

[Andrew Lunn]

On Fri, Aug 25, 2017 at 08:47:00AM -0500, Larry Finger wrote:
> On 08/24/2017 08:54 PM, Andrew Lunn wrote:
> >netdev frowns upon debugfs. You should try to keep this altogether,
> >making it easy to throw away before the driver is moved out of
> >staging.
> >
> >You might want to look at ethtool -d. That will be accepted.
> 
> Andrew,
> 
> What is the problem with debugfs?

You should probably look back in the discussions on the netdev
mailling list. But basically, anything you want to export should
follow generic well defined interface, which can be used by other
drivers. debugfs tends to be a mess, a wild west, each driver doing
its own thing, not standardisation. It is O.K. for your own
development work, you can have your own out of tree patches adding in
debugfs, but such patches are unlikely to be accepted into mainline.
David has threatened to simply rip out all debugfs code from all
network drivers. There is push back on adding any new debugfs code,
and some driver writers have taken out debugfs code in their own
drivers, often replacing it with something generic all drivers can
use.
 
> Please suggest which driver has the best example of an ethtool -d
> implementation that we might study.

The API is very simple. In your ethtool_ops you need to function:
.get_regs_len, returns an int, the number of registers you can dump.
.get_regs returns the contents.

There are plenty of examples, e.g.:

http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/amd/pcnet32.c#L1436
 
If you need something more structured, look around, see if other
drivers have similar needs, and propose something generic.

> The first version of the debugfs changes were sent to wireless-drivers on
> July 2. Why are we first hearing of this objection nearly 2 months later?

Different reviewers tend to review different things. I personally
don't care so much who the vendor is, but look out for some specific
things i feel qualified to review. Ethernet PHYs and MDIO drivers,
drivers which make use of Ethernet PHYs, APIs which allow userspace to
write to registers, debugfs, changing MTUs, etc.

There are plenty of patches on netdev for me to review, so i don't
cast a wider net to other lists, like for example wireless-drivers.
You would of got this comment sooner or later, since you should be
posting to netdev at some point anyway. I can also imaging there is
also some reluctance to review staging code, until it is getting close
to moving out of staging.

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

Re: [PATCH] memstick: rtsx: fix ms card data transfer bug

[Andrew Morton]

On Wed, 30 Oct 2013 14:40:16 +0800  wrote:

> unlike mspro card, ms card use normal read/write mode for DMA
> data transfer.

What are the user-visible effects of this bug?

Please always include this information when fixing bugs so that others
can decide whether they (or their customers) need the patch.

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

Re: [PATCH] memstick: rtsx: fix ms card data transfer bug

[Andrew Morton]

On Wed, 6 Nov 2013 09:14:59 +0800 micky  wrote:

> On 11/06/2013 05:10 AM, Andrew Morton wrote:
> > On Wed, 30 Oct 2013 14:40:16 +0800  wrote:
> >
> >> unlike mspro card, ms card use normal read/write mode for DMA
> >> data transfer.
> > What are the user-visible effects of this bug?
> >
> > Please always include this information when fixing bugs so that others
> > can decide whether they (or their customers) need the patch.
> >
>

(top-posting repaired - please don't top-post!)

> MS card can not use auto read/write mode, so it will fail at
> initialize and long data transfer. This patch is used to add
> support for ms card.
> 
> Shall I re-send this patch to add more info?

That's OK - I updated the changelog in-place and added cc:stable so it
gets backported.  But then I dropped the patch ;)

>From this info I assume that use of ms cards is very rare, otherwise
people would have complained.  What is the difference between an "ms
card" and an "mspro card"?  How common are each type and what is their
availability?

ms_transfer_data() and mspro_transfer_data() are very similar.  I think
it would be more maintainable if they were integrated into a single
function?

trans_done and timeleft could be made local to the code block where
they used.  This would be neater and more maintainable.

This code is troublesome:

:   if (pcr->trans_result == TRANS_NOT_READY) {
:   init_completion(&trans_done);
:   timeleft = wait_for_completion_interruptible_timeout(
:   &trans_done, 1000);
:   if (timeleft < 0) {
:   dev_dbg(ms_dev(host),
:   "%s: timeout wait for ok interrupt.\n",
:   __func__);
:   return -ETIMEDOUT;
:   }
:   }

- Why does it exist?  Needs a comment explaining what it is trying to
  achieve.

- It should use DECLARE_COMPLETION_ONSTACK() for trans_done

- It uses wait_for_completion() but nothing ever calls complete() on
  the object!  That's just bizarre and more appropriate primitives
  should be used.

- The debug message is hard to understand and appears to be wrong. 
  Should be "interrupt received while waiting for ".

- The code appears to be terminating a kernel IO transaction when the
  user hits ^C.  That's just not viable - the ^C could have been
  entered for other reasons and the IO will complete just fine.  Also
  no -EINTR is returned callers don't appear to be set up to handle it.

So shudder.  I'll drop the patch.  Please explain very carefully what
you're trying to achieve here and perhaps we can suggest a suitable
implementation approach.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] checkpatch: Improve missing blank line after declarations test

[Andrew Morton]

On Mon, 05 May 2014 13:12:16 -0700 Joe Perches  wrote:

> A couple more modifications to the declarations tests.
> 
> o Declarations can also be bitfields so exclude things with a colon
> o Make sure the current and previous lines are indented the same
>   to avoid matching some macro where a struct type is passed on
>   the previous line like:
> 
>   next = list_entry(buffer->entry.next,
> struct binder_buffer, entry);
>   if (buffer_start_page(next) == buffer_end_page(buffer)) 

So 
checkpatch-always-warn-on-missing-blank-line-after-variable-declaration-block.patch
is stuck in -mm while I evaluate its effects.  Thus far that evaluation
has been "super non-intrusive", because the patch doesn't actually
do anything.

--- a/fs/open.c~a
+++ a/fs/open.c
@@ -39,6 +39,7 @@ int do_truncate(struct dentry *dentry, l
 {
int ret;
struct iattr newattrs;
+   wibble();
 
/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
if (length < 0)
@@ -67,6 +68,7 @@ long vfs_truncate(struct path *path, lof
 {
struct inode *inode;
long error;
+   wobble();
 
inode = path->dentry->d_inode;
 



I add --strict and it still doesn't warn.  What did I do wrong this time?

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

Re: [PATCH] checkpatch: Improve missing blank line after declarations test

[Andrew Morton]

On Mon, 05 May 2014 15:35:43 -0700 Joe Perches  wrote:

> > @@ -67,6 +68,7 @@ long vfs_truncate(struct path *path, lof
> >  {
> > struct inode *inode;
> > long error;
> > +   wobble();
> >  
> > inode = path->dentry->d_inode;
> 
> Patch content can be a bit odd when lines are
> both added and deleted so checkpatch bleats
> only when both lines are added.
> 
> + int foo;
> + wibble();
> 
> generates a complaint.
> 
>   int foo;
> + wibble_wobble();
> 
> does not.

Oh, OK.

I have seen no instances of this warning since adding the patch.  So I
guess it's safe to merge but perhaps insufficiently aggressive.  Or
maybe people are being well-behaved.

Oh well, I'll keep an eye out.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] list: Fix order of arguments for hlist_add_after(_rcu)

[Andrew Morton]

On Fri, 6 Jun 2014 10:22:51 -0700 "Paul E. McKenney" 
 wrote:

> On Fri, Jun 06, 2014 at 03:56:52PM +, David Laight wrote:
> > From: Behalf Of Ken Helias
> > > All other add functions for lists have the new item as first argument and 
> > > the
> > > position where it is added as second argument. This was changed for no 
> > > good
> > > reason in this function and makes using it unnecessary confusing.
> > > 
> > > Also the naming of the arguments in hlist_add_after was confusing. It was
> > > changed to use the same names as hlist_add_after_rcu.
> > ...
> > > -static inline void hlist_add_after_rcu(struct hlist_node *prev,
> > > -struct hlist_node *n)
> > > +static inline void hlist_add_after_rcu(struct hlist_node *n,
> > > +struct hlist_node *prev)
> > 
> > It is rather a shame that the change doesn't generate a compilation
> > error for old source files.
> 
> I am also a bit concerned by this.
> 

yup.  hlist_add_behind()?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] lib / string_helpers: clean up test suite

[Andrew Morton]

On Wed,  2 Jul 2014 16:20:24 +0300 Andy Shevchenko 
 wrote:

> This patch prepares test suite for a following update. It introduces
> test_string_check_buf() helper which checks the result and dumps an error.
> 
> ...
>
> --- a/lib/test-string_helpers.c
> +++ b/lib/test-string_helpers.c
> @@ -10,6 +10,26 @@
>  #include 
>  #include 
>  
> +static __init bool test_string_check_buf(const char *name, unsigned int 
> flags,
> +  char *in, size_t p,
> +  char *out_real, size_t q_real,
> +  char *out_test, size_t q_test)
> +{
> + if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> + return true;
> +
> + pr_err("Test '%s' failed: flags = %u\n", name, flags);
> +
> + print_hex_dump(KERN_WARNING, "Input: ", DUMP_PREFIX_NONE, 16, 1,
> +in, p, true);
> + print_hex_dump(KERN_WARNING, "Expected: ", DUMP_PREFIX_NONE, 16, 1,
> +out_test, q_test, true);
> + print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
> +out_real, q_real, true);

Seems strange to mix KERN_ERR and KERN_WARNING.  The code's always been
that way, but maybe it can be improved.


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

Re: [PATCH 2/6] lib / string_helpers: introduce string_escape_mem()

[Andrew Morton]

On Wed,  2 Jul 2014 16:20:25 +0300 Andy Shevchenko 
 wrote:

> This is almost the opposite function to string_unescape(). Nevertheless it
> handles \0 and could be used for any byte buffer.
> 
> The documentation is supplied together with the function prototype.
> 
> The test cases covers most of the scenarios and would be expanded later on.
> 
> ...
>
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -71,4 +71,87 @@ static inline int string_unescape_any_inplace(char *buf)
>   return string_unescape_any(buf, buf, 0);
>  }
>  
> +#define ESCAPE_SPACE 0x01
> +#define ESCAPE_SPECIAL   0x02
> +#define ESCAPE_NULL  0x04
> +#define ESCAPE_OCTAL 0x08
> +#define ESCAPE_ANY   \
> + (ESCAPE_SPACE | ESCAPE_OCTAL | ESCAPE_SPECIAL | ESCAPE_NULL)
> +#define ESCAPE_NP0x10
> +#define ESCAPE_ANY_NP(ESCAPE_ANY | ESCAPE_NP)
> +#define ESCAPE_HEX   0x20
> +
> +/**
> + * string_escape_mem - quote characters in the given memory buffer

It drive me nuts when the kerneldoc is in the .h file.  Who thinks of
looking there?  I realise that string_unescape() already did that, but
I'd prefer that we fix string_unescape() rather than imitate it.

> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c

This is a lot of code!  Adds nearly a kbyte.  I'm surprised that
escaping a string is so verbose.

I wonder if the implementation really needs to be so comprehensive?

Would a table-driven approach be more compact?

>  static int __init test_string_helpers_init(void)
>  {
>   unsigned int i;
> @@ -112,6 +315,16 @@ static int __init test_string_helpers_init(void)
>   test_string_unescape("unescape inplace",
>get_random_int() % (UNESCAPE_ANY + 1), true);
>  
> + /* Without dictionary */
> + for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
> + test_string_escape("escape 0", escape0, i, 
> TEST_STRING_2_DICT_0);
> +
> + /* With dictionary */
> + for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
> + test_string_escape("escape 1", escape1, i, 
> TEST_STRING_2_DICT_1);
> +
> + test_string_escape_nomem();
> +
>   return -EINVAL;
>  }

I wonder why this returns -EINVAL.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 00/11] lib: introduce string_escape_mem and %*pE specifier

[Andrew Morton]

On Thu, 28 Aug 2014 14:33:30 -0400 "John W. Linville"  
wrote:

> On Wed, Aug 20, 2014 at 12:42:41PM +0300, Andy Shevchenko wrote:
> > The introduced function is a kind of opposite to string_unescape. We have
> > several users of such functionality each of them created custom 
> > implementation.
> > The series contains clean up of test suite, adding new call, and switching 
> > few
> > users to use it via %*pE specifier.
>
> Any objections?  Do you (Andy) want me to merge this through the
> wireless tree?
> 

Who is this top-poster and what have you done with John?

I was going to apply them yesterday but in [4/11] Andy said he plans on
sending out a v5. (v4 actually?)

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

Re: [PATCH v3 00/11] lib: introduce string_escape_mem and %*pE specifier

[Andrew Morton]

On Thu, 28 Aug 2014 23:58:45 +0300 Andy Shevchenko  
wrote:

> On Thu, Aug 28, 2014 at 10:08 PM, Andrew Morton
>  wrote:
> > On Thu, 28 Aug 2014 14:33:30 -0400 "John W. Linville" 
> >  wrote:
> >
> >> On Wed, Aug 20, 2014 at 12:42:41PM +0300, Andy Shevchenko wrote:
> >> > The introduced function is a kind of opposite to string_unescape. We have
> >> > several users of such functionality each of them created custom 
> >> > implementation.
> >> > The series contains clean up of test suite, adding new call, and 
> >> > switching few
> >> > users to use it via %*pE specifier.
> >>
> >> Any objections?  Do you (Andy) want me to merge this through the
> >> wireless tree?
> >>
> >
> > Who is this top-poster and what have you done with John?
> >
> > I was going to apply them yesterday but in [4/11] Andy said he plans on
> > sending out a v5. (v4 actually?)
> 
> For now (so far no more comments) it is only couple of trivia fixes
> (removing useless comments). Would you like to resend whole series or
> just fixup would be enough?

I fixed up the one Joe commented on.

--- a/lib/vsprintf.c~lib-vsprintf-add-%pe-format-specifier-fix
+++ a/lib/vsprintf.c
@@ -,12 +,11 @@ char *escaped_string(char *buf, char *en
int len;
 
if (spec.field_width == 0)
-   /* nothing to print */
-   return buf;
+   return buf; /* nothing to print */
 
if (ZERO_OR_NULL_PTR(addr))
-   /* NULL pointer */
-   return string(buf, end, NULL, spec);
+   return string(buf, end, NULL, spec);/* NULL pointer */
+
 
do {
switch (fmt[count++]) {
_

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

[PATCH] Staging: bcm: LeakyBucket: format kernel-docs

[Andrew Plummer]

Remove insignificant spaces before tabs in comments.

Signed-off-by: Andrew Plummer 
---
 drivers/staging/bcm/LeakyBucket.c | 81 ---
 1 file changed, 33 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/bcm/LeakyBucket.c 
b/drivers/staging/bcm/LeakyBucket.c
index 8c4030d..d6b55f9 100644
--- a/drivers/staging/bcm/LeakyBucket.c
+++ b/drivers/staging/bcm/LeakyBucket.c
@@ -1,20 +1,16 @@
 /**
-*  LEAKYBUCKET.C
+*  LEAKYBUCKET.C
 *  This file contains the routines related to Leaky Bucket Algorithm.
 ***/
 #include "headers.h"
 
-/*
-* Function- UpdateTokenCount()
-*
-* Description - This function calculates the token count for each
-*  channel and updates the same in Adapter 
strucuture.
-*
-* Parameters  - Adapter: Pointer to the Adapter structure.
-*
-* Returns - None
-**/
-
+/**
+ * UpdateTokenCount() - Calculates the token count for each channel
+ * and updates the same in Adapter structure
+ * @Adapter:   Pointer to the Adapter structure.
+ *
+ * Return: None
+ */
 static VOID UpdateTokenCount(register struct bcm_mini_adapter *Adapter)
 {
ULONG liCurrentTime;
@@ -59,20 +55,16 @@ static VOID UpdateTokenCount(register struct 
bcm_mini_adapter *Adapter)
 }
 
 
-/*
-* Function- IsPacketAllowedForFlow()
-*
-* Description - This function checks whether the given packet from the
-*  specified queue can be allowed for transmission 
by
-*  checking the token count.
-*
-* Parameters  - Adapter  : Pointer to the Adpater structure.
-*- iQIndex   : The queue Identifier.
-*- ulPacketLength: Number of bytes to be 
transmitted.
-*
-* Returns - The number of bytes allowed for transmission.
-*
-***/
+/**
+ * IsPacketAllowedForFlow() - This function checks whether the given
+ * packet from the specified queue can be allowed for transmission by
+ * checking the token count.
+ * @Adapter:   Pointer to the Adpater structure.
+ * @iQIndex:   The queue Identifier.
+ * @ulPacketLength:Number of bytes to be transmitted.
+ *
+ * Returns: The number of bytes allowed for transmission.
+ */
 static ULONG GetSFTokenCount(struct bcm_mini_adapter *Adapter, struct 
bcm_packet_info *psSF)
 {
BCM_DEBUG_PRINT(Adapter, DBG_TYPE_TX, TOKEN_COUNTS, DBG_LVL_ALL,
@@ -256,18 +248,14 @@ static void send_control_packet(struct bcm_mini_adapter 
*ad,
}
 }
 
-/
-* Function- CheckAndSendPacketFromIndex()
-*
-* Description - This function dequeues the data/control packet from the
-*  specified queue for transmission.
-*
-* Parameters  - Adapter : Pointer to the driver control structure.
-*- iQIndex : The queue Identifier.
-*
-* Returns - None.
-*
-/
+/**
+ * CheckAndSendPacketFromIndex() - This function dequeues the
+ * data/control packet from the specified queue for transmission.
+ * @Adapter:   Pointer to the driver control structure.
+ * @iQIndex:   The queue Identifier.
+ *
+ * Returns: None.
+ */
 static VOID CheckAndSendPacketFromIndex(struct bcm_mini_adapter *Adapter,
struct bcm_packet_info *psSF)
 {
@@ -284,16 +272,13 @@ static VOID CheckAndSendPacketFromIndex(struct 
bcm_mini_adapter *Adapter,
 }
 
 
-/***
-* Function- transmit_packets()
-*
-* Description - This function transmits the packets from different
-*  queues, if free descriptors are available on 
target.
-*
-* Parameters  - Adapter:  Pointer to the Adapter structure.
-*
-* Returns - None.
-/
+/**
+ * transmit_packets() - This function transmits the packets from
+ * different queues, if free descriptors are available on target.
+ * @Adapter:   Pointer to the Adapter structure.
+ *
+ * Returns: None.
+ */
 VOID transmit_packets(struct bcm_mini_adapter *Adapter)
 {
UINT uiPrevTotalCount = 0;
-- 
1.9.1

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

[PATCH] Staging: nokia_h4p: nokia_fw: remove extra return

[Andrew Plummer]

Remove empty return at end of function.

Signed-off-by: Andrew Plummer 
---
 drivers/staging/nokia_h4p/nokia_fw.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/nokia_h4p/nokia_fw.c 
b/drivers/staging/nokia_h4p/nokia_fw.c
index 14ba219..18953ae 100644
--- a/drivers/staging/nokia_h4p/nokia_fw.c
+++ b/drivers/staging/nokia_h4p/nokia_fw.c
@@ -197,8 +197,6 @@ void hci_h4p_parse_fw_event(struct hci_h4p_info *info, 
struct sk_buff *skb)
dev_err(info->dev, "Don't know how to parse fw event\n");
info->fw_error = -EINVAL;
}
-
-   return;
 }
 
 MODULE_FIRMWARE(FW_NAME_TI1271_PRELE);
-- 
1.9.1

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

[PATCH] Staging: emxx_udc: emxx_udc: remove spaces before semicolons

[Andrew Plummer]

Remove spaces before semicolons to remove checkpatch warnings.

Signed-off-by: Andrew Plummer 
---
 drivers/staging/emxx_udc/emxx_udc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/emxx_udc/emxx_udc.c 
b/drivers/staging/emxx_udc/emxx_udc.c
index b2eaf01..adc24a9 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -77,14 +77,14 @@ struct nbu2ss_udc udc_controller;
 /* Read */
 static inline u32 _nbu2ss_readl(void *address)
 {
-   return __raw_readl(address) ;
+   return __raw_readl(address);
 }
 
 /*-*/
 /* Write */
 static inline void _nbu2ss_writel(void *address, u32 udata)
 {
-   __raw_writel(udata, address) ;
+   __raw_writel(udata, address);
 }
 
 /*-*/
@@ -92,7 +92,7 @@ static inline void _nbu2ss_writel(void *address, u32 udata)
 static inline void _nbu2ss_bitset(void *address, u32 udata)
 {
u32 reg_dt = __raw_readl(address) | (udata);
-   __raw_writel(reg_dt, address) ;
+   __raw_writel(reg_dt, address);
 }
 
 /*-*/
@@ -100,7 +100,7 @@ static inline void _nbu2ss_bitset(void *address, u32 udata)
 static inline void _nbu2ss_bitclr(void *address, u32 udata)
 {
u32 reg_dt = __raw_readl(address) & ~(udata);
-   __raw_writel(reg_dt, address) ;
+   __raw_writel(reg_dt, address);
 }
 
 #ifdef UDC_DEBUG_DUMP
-- 
1.9.1

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

Re: [PATCH 21/44] power/reset: gpio-poweroff: Register with kernel poweroff handler

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:23PM -0700, Guenter Roeck wrote:
> Register with kernel poweroff handler instead of setting pm_power_off
> directly. Register with a low priority value of 64 to reflect that
> the original code only sets pm_power_off if it was not already set.
> 
> Other changes:
> 
> Drop note that there can not be an additional instance of this driver.
> The original reason no longer applies, it should be obvious that there
> can only be one instance of the driver if static variables are used to
> reflect its state, and support for multiple instances can now be added
> easily if needed by avoiding static variables.
> 
> Do not create an error message if another poweroff handler has already been
> registered. This is perfectly normal and acceptable.
> 
> Do not display a warning traceback if the poweroff handler fails to
> power off the system. There may be other poweroff handlers.

I would prefer to keep the warning traceback. We found on some
hardware the GPIO transitions were too fast and it failed to power
off. Seeing the traceback gives an idea where to go look for the
problem.

Other than that,

Acked-by: Andrew Lunn 

> 
> Cc: Sebastian Reichel 
> Cc: Dmitry Eremin-Solenikov 
> Cc: David Woodhouse 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/power/reset/gpio-poweroff.c | 36 ++--
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/power/reset/gpio-poweroff.c 
> b/drivers/power/reset/gpio-poweroff.c
> index ce849bc..e95a7a1 100644
> --- a/drivers/power/reset/gpio-poweroff.c
> +++ b/drivers/power/reset/gpio-poweroff.c
> @@ -14,18 +14,18 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  
> -/*
> - * Hold configuration here, cannot be more than one instance of the driver
> - * since pm_power_off itself is global.
> - */
>  static struct gpio_desc *reset_gpio;
>  
> -static void gpio_poweroff_do_poweroff(void)
> +static int gpio_poweroff_do_poweroff(struct notifier_block *this,
> +  unsigned long unused1, void *unused2)
> +
>  {
>   BUG_ON(!reset_gpio);
>  
> @@ -42,20 +42,18 @@ static void gpio_poweroff_do_poweroff(void)
>   /* give it some time */
>   mdelay(3000);
>  
> - WARN_ON(1);
> + return NOTIFY_DONE;
>  }
>  
> +static struct notifier_block gpio_poweroff_nb = {
> + .notifier_call = gpio_poweroff_do_poweroff,
> + .priority = 64,
> +};
> +
>  static int gpio_poweroff_probe(struct platform_device *pdev)
>  {
>   bool input = false;
> -
> - /* If a pm_power_off function has already been added, leave it alone */
> - if (pm_power_off != NULL) {
> - dev_err(&pdev->dev,
> - "%s: pm_power_off function already registered",
> -__func__);
> - return -EBUSY;
> - }
> + int err;
>  
>   reset_gpio = devm_gpiod_get(&pdev->dev, NULL);
>   if (IS_ERR(reset_gpio))
> @@ -77,14 +75,16 @@ static int gpio_poweroff_probe(struct platform_device 
> *pdev)
>   }
>   }
>  
> - pm_power_off = &gpio_poweroff_do_poweroff;
> - return 0;
> + err = register_poweroff_handler(&gpio_poweroff_nb);
> + if (err)
> + dev_err(&pdev->dev, "Failed to register poweroff handler\n");
> +
> + return err;
>  }
>  
>  static int gpio_poweroff_remove(struct platform_device *pdev)
>  {
> - if (pm_power_off == &gpio_poweroff_do_poweroff)
> - pm_power_off = NULL;
> + unregister_poweroff_handler(&gpio_poweroff_nb);
>  
>   return 0;
>  }
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 23/44] power/reset: qnap-poweroff: Register with kernel poweroff handler

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:25PM -0700, Guenter Roeck wrote:
> Register with kernel poweroff handler instead of setting pm_power_off
> directly. Register with default priority value of 128 to reflect that
> the original code generates an error if another poweroff handler has
> already been registered when the driver is loaded.
> 
> Cc: Sebastian Reichel 
> Cc: Dmitry Eremin-Solenikov 
> Cc: David Woodhouse 
> Signed-off-by: Guenter Roeck 

Acked-by: Andrew Lunn 

Thanks
Andrew

> ---
>  drivers/power/reset/qnap-poweroff.c | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/power/reset/qnap-poweroff.c 
> b/drivers/power/reset/qnap-poweroff.c
> index a75db7f..c474980 100644
> --- a/drivers/power/reset/qnap-poweroff.c
> +++ b/drivers/power/reset/qnap-poweroff.c
> @@ -16,7 +16,9 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -55,7 +57,8 @@ static void __iomem *base;
>  static unsigned long tclk;
>  static const struct power_off_cfg *cfg;
>  
> -static void qnap_power_off(void)
> +static int qnap_power_off(struct notifier_block *this, unsigned long unused1,
> +   void *unused2)
>  {
>   const unsigned divisor = ((tclk + (8 * cfg->baud)) / (16 * cfg->baud));
>  
> @@ -72,14 +75,20 @@ static void qnap_power_off(void)
>  
>   /* send the power-off command to PIC */
>   writel(cfg->cmd, UART1_REG(TX));
> +
> + return NOTIFY_DONE;
>  }
>  
> +static struct notifier_block qnap_poweroff_nb = {
> + .notifier_call = qnap_power_off,
> + .priority = 128,
> +};
> +
>  static int qnap_power_off_probe(struct platform_device *pdev)
>  {
>   struct device_node *np = pdev->dev.of_node;
>   struct resource *res;
>   struct clk *clk;
> - char symname[KSYM_NAME_LEN];
>  
>   const struct of_device_id *match =
>   of_match_node(qnap_power_off_of_match_table, np);
> @@ -106,22 +115,13 @@ static int qnap_power_off_probe(struct platform_device 
> *pdev)
>  
>   tclk = clk_get_rate(clk);
>  
> - /* Check that nothing else has already setup a handler */
> - if (pm_power_off) {
> - lookup_symbol_name((ulong)pm_power_off, symname);
> - dev_err(&pdev->dev,
> - "pm_power_off already claimed %p %s",
> - pm_power_off, symname);
> - return -EBUSY;
> - }
> - pm_power_off = qnap_power_off;
> -
> - return 0;
> + return register_poweroff_handler(&qnap_poweroff_nb);
>  }
>  
>  static int qnap_power_off_remove(struct platform_device *pdev)
>  {
> - pm_power_off = NULL;
> + unregister_poweroff_handler(&qnap_poweroff_nb);
> +
>   return 0;
>  }
>  
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 20/44] power/reset: restart-poweroff: Register with kernel poweroff handler

[Andrew Lunn]

On Mon, Oct 06, 2014 at 10:28:22PM -0700, Guenter Roeck wrote:
> Register with kernel poweroff handler instead of seting pm_power_off
> directly.  Register as poweroff handler of last resort since the driver
> does not really power off the system but executes a restart.

I would not say last resort, this is how it is designed to work. There
is no way to turn the power off from with linux, it is designed that
u-boot will put the hardware into minimal power consumption until the
"power" button is pressed.

Other than that, 

Acked-by: Andrew Lunn 

Thanks
Andrew

> 
> Cc: Sebastian Reichel 
> Cc: Dmitry Eremin-Solenikov 
> Cc: David Woodhouse 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/power/reset/restart-poweroff.c | 25 -
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/reset/restart-poweroff.c 
> b/drivers/power/reset/restart-poweroff.c
> index edd707e..5437697 100644
> --- a/drivers/power/reset/restart-poweroff.c
> +++ b/drivers/power/reset/restart-poweroff.c
> @@ -12,35 +12,34 @@
>   */
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> -#include 
>  
> -static void restart_poweroff_do_poweroff(void)
> +static int restart_poweroff_do_poweroff(struct notifier_block *this,
> + unsigned long unused1, void *unused2)
>  {
>   reboot_mode = REBOOT_HARD;
>   machine_restart(NULL);
> +
> + return NOTIFY_DONE;
>  }
>  
> +static struct notifier_block restart_poweroff_handler = {
> + .notifier_call = restart_poweroff_do_poweroff,
> +};
> +
>  static int restart_poweroff_probe(struct platform_device *pdev)
>  {
> - /* If a pm_power_off function has already been added, leave it alone */
> - if (pm_power_off != NULL) {
> - dev_err(&pdev->dev,
> - "pm_power_off function already registered");
> - return -EBUSY;
> - }
> -
> - pm_power_off = &restart_poweroff_do_poweroff;
> - return 0;
> + return register_poweroff_handler(&restart_poweroff_handler);
>  }
>  
>  static int restart_poweroff_remove(struct platform_device *pdev)
>  {
> - if (pm_power_off == &restart_poweroff_do_poweroff)
> - pm_power_off = NULL;
> + unregister_poweroff_handler(&restart_poweroff_handler);
>  
>   return 0;
>  }
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel