RE: [PATCH net 1/2] net: hns3: Remove the left over redundant check & assignment
> From: Leon Romanovsky [mailto:l...@kernel.org]
> Sent: Monday, April 5, 2021 1:43 PM
> To: Salil Mehta
> Cc: da...@davemloft.net; k...@kernel.org; net...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Linuxarm ;
> linux...@openeuler.org
> Subject: Re: [PATCH net 1/2] net: hns3: Remove the left over redundant check
> & assignment
>
> On Mon, Apr 05, 2021 at 12:26:37PM +, Salil Mehta wrote:
> > Hi Leon,
> > Thanks for the review.
> >
> > > From: Leon Romanovsky [mailto:l...@kernel.org]
> > > Sent: Sunday, April 4, 2021 7:26 AM
> > > To: Salil Mehta
> > > Cc: da...@davemloft.net; k...@kernel.org; net...@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Linuxarm ;
> > > linux...@openeuler.org
> > > Subject: Re: [PATCH net 1/2] net: hns3: Remove the left over redundant
> > > check
> > > & assignment
> > >
> > > On Sat, Apr 03, 2021 at 02:35:19AM +0100, Salil Mehta wrote:
> > > > This removes the left over check and assignment which is no longer used
> > > > anywhere in the function and should have been removed as part of the
> > > > below mentioned patch.
> > > >
> > > > Fixes: 012fcb52f67c ("net: hns3: activate reset timer when calling
> > > reset_event")
> > > > Signed-off-by: Salil Mehta
> > > > ---
> > > > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 --
> > > > 1 file changed, 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > > b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > > > index e3f81c7e0ce7..7ad0722383f5 100644
> > > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > > > @@ -3976,8 +3976,6 @@ static void hclge_reset_event(struct pci_dev
> > > > *pdev,
> > > struct hnae3_handle *handle)
> > > > * want to make sure we throttle the reset request. Therefore,
> > > > we will
> > > > * not allow it again before 3*HZ times.
> > > > */
> > > > - if (!handle)
> > > > - handle = >vport[0].nic;
> > >
> > > The comment above should be updated too, and probably the signature of
> > > hclge_reset_event() worth to be changed.
> >
> >
> > Yes, true. Both the comment and the prototype will be updated in near
> > future.
> > I can assure you this did not go un-noticed during the change. There are
> > some internal subtleties which I am trying to sort out. Those might come
> > as part of different patch-set which deals with other related changes as
> > well.
>
> I can buy such explanation for the change in function signature, but have hard
> time to believe that extra commit is needed to change comment above.
Sure, I understand your point. Earlier I thought to retain the comment for
reference
and make it part of the refactor change. But will send another version changing
the comment now. No issues :)
Many thanks
>
> Thanks
>
> >
> > The current change(and some other) will pave the way for necessary
> > refactoring
> > Of the code being done.
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > if (time_before(jiffies, (hdev->last_reset_time +
> > > > HCLGE_RESET_INTERVAL))) {
> > > > --
> > > > 2.17.1
> > > >
Re: [PATCH net 1/2] net: hns3: Remove the left over redundant check & assignment
On Mon, Apr 05, 2021 at 12:26:37PM +, Salil Mehta wrote:
> Hi Leon,
> Thanks for the review.
>
> > From: Leon Romanovsky [mailto:l...@kernel.org]
> > Sent: Sunday, April 4, 2021 7:26 AM
> > To: Salil Mehta
> > Cc: da...@davemloft.net; k...@kernel.org; net...@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Linuxarm ;
> > linux...@openeuler.org
> > Subject: Re: [PATCH net 1/2] net: hns3: Remove the left over redundant check
> > & assignment
> >
> > On Sat, Apr 03, 2021 at 02:35:19AM +0100, Salil Mehta wrote:
> > > This removes the left over check and assignment which is no longer used
> > > anywhere in the function and should have been removed as part of the
> > > below mentioned patch.
> > >
> > > Fixes: 012fcb52f67c ("net: hns3: activate reset timer when calling
> > reset_event")
> > > Signed-off-by: Salil Mehta
> > > ---
> > > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > > index e3f81c7e0ce7..7ad0722383f5 100644
> > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > > @@ -3976,8 +3976,6 @@ static void hclge_reset_event(struct pci_dev *pdev,
> > struct hnae3_handle *handle)
> > >* want to make sure we throttle the reset request. Therefore, we will
> > >* not allow it again before 3*HZ times.
> > >*/
> > > - if (!handle)
> > > - handle = >vport[0].nic;
> >
> > The comment above should be updated too, and probably the signature of
> > hclge_reset_event() worth to be changed.
>
>
> Yes, true. Both the comment and the prototype will be updated in near future.
> I can assure you this did not go un-noticed during the change. There are
> some internal subtleties which I am trying to sort out. Those might come
> as part of different patch-set which deals with other related changes as well.
I can buy such explanation for the change in function signature, but have hard
time to believe that extra commit is needed to change comment above.
Thanks
>
> The current change(and some other) will pave the way for necessary refactoring
> Of the code being done.
>
>
> >
> > Thanks
> >
> > >
> > > if (time_before(jiffies, (hdev->last_reset_time +
> > > HCLGE_RESET_INTERVAL))) {
> > > --
> > > 2.17.1
> > >
RE: [PATCH net 1/2] net: hns3: Remove the left over redundant check & assignment
Hi Leon,
Thanks for the review.
> From: Leon Romanovsky [mailto:l...@kernel.org]
> Sent: Sunday, April 4, 2021 7:26 AM
> To: Salil Mehta
> Cc: da...@davemloft.net; k...@kernel.org; net...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Linuxarm ;
> linux...@openeuler.org
> Subject: Re: [PATCH net 1/2] net: hns3: Remove the left over redundant check
> & assignment
>
> On Sat, Apr 03, 2021 at 02:35:19AM +0100, Salil Mehta wrote:
> > This removes the left over check and assignment which is no longer used
> > anywhere in the function and should have been removed as part of the
> > below mentioned patch.
> >
> > Fixes: 012fcb52f67c ("net: hns3: activate reset timer when calling
> reset_event")
> > Signed-off-by: Salil Mehta
> > ---
> > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > index e3f81c7e0ce7..7ad0722383f5 100644
> > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> > @@ -3976,8 +3976,6 @@ static void hclge_reset_event(struct pci_dev *pdev,
> struct hnae3_handle *handle)
> > * want to make sure we throttle the reset request. Therefore, we will
> > * not allow it again before 3*HZ times.
> > */
> > - if (!handle)
> > - handle = >vport[0].nic;
>
> The comment above should be updated too, and probably the signature of
> hclge_reset_event() worth to be changed.
Yes, true. Both the comment and the prototype will be updated in near future.
I can assure you this did not go un-noticed during the change. There are
some internal subtleties which I am trying to sort out. Those might come
as part of different patch-set which deals with other related changes as well.
The current change(and some other) will pave the way for necessary refactoring
Of the code being done.
>
> Thanks
>
> >
> > if (time_before(jiffies, (hdev->last_reset_time +
> > HCLGE_RESET_INTERVAL))) {
> > --
> > 2.17.1
> >
Re: [PATCH net 1/2] net: hns3: Remove the left over redundant check & assignment
On Sat, Apr 03, 2021 at 02:35:19AM +0100, Salil Mehta wrote:
> This removes the left over check and assignment which is no longer used
> anywhere in the function and should have been removed as part of the
> below mentioned patch.
>
> Fixes: 012fcb52f67c ("net: hns3: activate reset timer when calling
> reset_event")
> Signed-off-by: Salil Mehta
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index e3f81c7e0ce7..7ad0722383f5 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -3976,8 +3976,6 @@ static void hclge_reset_event(struct pci_dev *pdev,
> struct hnae3_handle *handle)
>* want to make sure we throttle the reset request. Therefore, we will
>* not allow it again before 3*HZ times.
>*/
> - if (!handle)
> - handle = >vport[0].nic;
The comment above should be updated too, and probably the signature of
hclge_reset_event() worth to be changed.
Thanks
>
> if (time_before(jiffies, (hdev->last_reset_time +
> HCLGE_RESET_INTERVAL))) {
> --
> 2.17.1
>
[PATCH net 1/2] net: hns3: Remove the left over redundant check & assignment
This removes the left over check and assignment which is no longer used
anywhere in the function and should have been removed as part of the
below mentioned patch.
Fixes: 012fcb52f67c ("net: hns3: activate reset timer when calling reset_event")
Signed-off-by: Salil Mehta
---
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index e3f81c7e0ce7..7ad0722383f5 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -3976,8 +3976,6 @@ static void hclge_reset_event(struct pci_dev *pdev,
struct hnae3_handle *handle)
* want to make sure we throttle the reset request. Therefore, we will
* not allow it again before 3*HZ times.
*/
- if (!handle)
- handle = >vport[0].nic;
if (time_before(jiffies, (hdev->last_reset_time +
HCLGE_RESET_INTERVAL))) {
--
2.17.1
RE: [PATCH net 1/1] xdp: fix xdp_return_frame() kernel BUG throw for page_pool memory model
>> diff --git a/net/core/xdp.c b/net/core/xdp.c >> index 05354976c1fc..4eaa28972af2 100644 >> --- a/net/core/xdp.c >> +++ b/net/core/xdp.c >> @@ -350,7 +350,8 @@ static void __xdp_return(void *data, struct >xdp_mem_info *mem, bool napi_direct, >> /* mem->id is valid, checked in >xdp_rxq_info_reg_mem_model() */ >> xa = rhashtable_lookup(mem_id_ht, >id, >mem_id_rht_params); >> page = virt_to_head_page(data); >> -napi_direct &= !xdp_return_frame_no_direct(); >> +if (napi_direct) >> +napi_direct &= !xdp_return_frame_no_direct(); > >if (napi_direct && xdp_return_frame_no_direct()) > napi_direct = false; > >I wonder if this code would be easier to understand? > Ya, I think this is more readable. Thanks. I will send v2.
Re: [PATCH net 1/1] xdp: fix xdp_return_frame() kernel BUG throw for page_pool memory model
Jesper Dangaard Brouer writes:
> On Mon, 29 Mar 2021 16:00:39 +0800
> Ong Boon Leong wrote:
>
>> xdp_return_frame() may be called outside of NAPI context to return
>> xdpf back to page_pool. xdp_return_frame() calls __xdp_return() with
>> napi_direct = false. For page_pool memory model, __xdp_return() calls
>> xdp_return_frame_no_direct() unconditionally and below false negative
>> kernel BUG throw happened under preempt-rt build:
>>
>> [ 430.450355] BUG: using smp_processor_id() in preemptible [] code:
>> modprobe/3884
>> [ 430.451678] caller is __xdp_return+0x1ff/0x2e0
>> [ 430.452111] CPU: 0 PID: 3884 Comm: modprobe Tainted: G U E
>> 5.12.0-rc2+ #45
>>
>> So, this patch fixes the issue by adding "if (napi_direct)" condition
>> to skip calling xdp_return_frame_no_direct() if napi_direct = false.
>>
>> Fixes: 2539650fadbf ("xdp: Helpers for disabling napi_direct of
>> xdp_return_frame")
>> Signed-off-by: Ong Boon Leong
>> ---
>
> This looks correct to me.
>
> Acked-by: Jesper Dangaard Brouer
>
>
>> net/core/xdp.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 05354976c1fc..4eaa28972af2 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -350,7 +350,8 @@ static void __xdp_return(void *data, struct xdp_mem_info
>> *mem, bool napi_direct,
>> /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>> xa = rhashtable_lookup(mem_id_ht, >id, mem_id_rht_params);
>> page = virt_to_head_page(data);
>> -napi_direct &= !xdp_return_frame_no_direct();
>> +if (napi_direct)
>> +napi_direct &= !xdp_return_frame_no_direct();
>
> if (napi_direct && xdp_return_frame_no_direct())
> napi_direct = false;
>
> I wonder if this code would be easier to understand?
Yes, IMO it would! :)
-Toke
Re: [PATCH net 1/1] xdp: fix xdp_return_frame() kernel BUG throw for page_pool memory model
On Mon, 29 Mar 2021 16:00:39 +0800
Ong Boon Leong wrote:
> xdp_return_frame() may be called outside of NAPI context to return
> xdpf back to page_pool. xdp_return_frame() calls __xdp_return() with
> napi_direct = false. For page_pool memory model, __xdp_return() calls
> xdp_return_frame_no_direct() unconditionally and below false negative
> kernel BUG throw happened under preempt-rt build:
>
> [ 430.450355] BUG: using smp_processor_id() in preemptible [] code:
> modprobe/3884
> [ 430.451678] caller is __xdp_return+0x1ff/0x2e0
> [ 430.452111] CPU: 0 PID: 3884 Comm: modprobe Tainted: G U E
> 5.12.0-rc2+ #45
>
> So, this patch fixes the issue by adding "if (napi_direct)" condition
> to skip calling xdp_return_frame_no_direct() if napi_direct = false.
>
> Fixes: 2539650fadbf ("xdp: Helpers for disabling napi_direct of
> xdp_return_frame")
> Signed-off-by: Ong Boon Leong
> ---
This looks correct to me.
Acked-by: Jesper Dangaard Brouer
> net/core/xdp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 05354976c1fc..4eaa28972af2 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -350,7 +350,8 @@ static void __xdp_return(void *data, struct xdp_mem_info
> *mem, bool napi_direct,
> /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> xa = rhashtable_lookup(mem_id_ht, >id, mem_id_rht_params);
> page = virt_to_head_page(data);
> - napi_direct &= !xdp_return_frame_no_direct();
> + if (napi_direct)
> + napi_direct &= !xdp_return_frame_no_direct();
if (napi_direct && xdp_return_frame_no_direct())
napi_direct = false;
I wonder if this code would be easier to understand?
> page_pool_put_full_page(xa->page_pool, page, napi_direct);
> rcu_read_unlock();
> break;
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
[PATCH net 1/1] xdp: fix xdp_return_frame() kernel BUG throw for page_pool memory model
xdp_return_frame() may be called outside of NAPI context to return
xdpf back to page_pool. xdp_return_frame() calls __xdp_return() with
napi_direct = false. For page_pool memory model, __xdp_return() calls
xdp_return_frame_no_direct() unconditionally and below false negative
kernel BUG throw happened under preempt-rt build:
[ 430.450355] BUG: using smp_processor_id() in preemptible [] code:
modprobe/3884
[ 430.451678] caller is __xdp_return+0x1ff/0x2e0
[ 430.452111] CPU: 0 PID: 3884 Comm: modprobe Tainted: G U E
5.12.0-rc2+ #45
So, this patch fixes the issue by adding "if (napi_direct)" condition
to skip calling xdp_return_frame_no_direct() if napi_direct = false.
Fixes: 2539650fadbf ("xdp: Helpers for disabling napi_direct of
xdp_return_frame")
Signed-off-by: Ong Boon Leong
---
net/core/xdp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 05354976c1fc..4eaa28972af2 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -350,7 +350,8 @@ static void __xdp_return(void *data, struct xdp_mem_info
*mem, bool napi_direct,
/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
xa = rhashtable_lookup(mem_id_ht, >id, mem_id_rht_params);
page = virt_to_head_page(data);
- napi_direct &= !xdp_return_frame_no_direct();
+ if (napi_direct)
+ napi_direct &= !xdp_return_frame_no_direct();
page_pool_put_full_page(xa->page_pool, page, napi_direct);
rcu_read_unlock();
break;
--
2.25.1
Re: [PATCH net 1/4] virtchnl: Fix layout of RSS structures
On 3/27/2021 2:53 AM, Geert Uytterhoeven wrote:
Hi Samudrala,
On Fri, Mar 26, 2021 at 11:45 PM Samudrala, Sridhar
wrote:
On 3/26/2021 1:06 AM, Geert Uytterhoeven wrote:
On Thu, Mar 25, 2021 at 11:29 PM Tony Nguyen wrote:
From: Norbert Ciosek
Remove padding from RSS structures. Previous layout
could lead to unwanted compiler optimizations
in loops when iterating over key and lut arrays.
From an earlier private conversation with Mateusz, I understand the real
explanation is that key[] and lut[] must be at the end of the
structures, because they are used as flexible array members?
Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
Signed-off-by: Norbert Ciosek
Tested-by: Konrad Jankowski
Signed-off-by: Tony Nguyen
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -476,7 +476,6 @@ struct virtchnl_rss_key {
u16 vsi_id;
u16 key_len;
u8 key[1]; /* RSS hash key, packed bytes */
- u8 pad[1];
};
VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
@@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
u16 vsi_id;
u16 lut_entries;
u8 lut[1];/* RSS lookup table */
- u8 pad[1];
};
If you use a flexible array member, it should be declared without a size,
i.e.
u8 key[];
Everything else is (trying to) fool the compiler, and leading to undefined
behavior, and people (re)adding explicit padding.
This header file is shared across other OSes that use C++ that doesn't support
flexible arrays. So the structures in this file use an array of size 1 as a last
element to enable variable sized arrays.
I don't think it is accepted practice to have non-Linux-isms in
include/*linux*/avf/virtchnl.h header files. Moreover, using a size
of 1 is counter-intuitive for people used to Linux kernel development,
and may lead to off-by-one errors in calculation of sizes.
If you insist on ignoring the above, this definitely deserves a
comment next to the member's declaration.
Sure. We can add a comment indicating that these fields are used
variable sized arrays.
Thanks
Sridhar
Re: [PATCH net 1/4] virtchnl: Fix layout of RSS structures
Hi Samudrala,
On Fri, Mar 26, 2021 at 11:45 PM Samudrala, Sridhar
wrote:
> On 3/26/2021 1:06 AM, Geert Uytterhoeven wrote:
> > On Thu, Mar 25, 2021 at 11:29 PM Tony Nguyen
> > wrote:
> > From: Norbert Ciosek
> >
> > Remove padding from RSS structures. Previous layout
> > could lead to unwanted compiler optimizations
> > in loops when iterating over key and lut arrays.
> >
> > From an earlier private conversation with Mateusz, I understand the real
> > explanation is that key[] and lut[] must be at the end of the
> > structures, because they are used as flexible array members?
> >
> > Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
> > Signed-off-by: Norbert Ciosek
> > Tested-by: Konrad Jankowski
> > Signed-off-by: Tony Nguyen
> >
> > --- a/include/linux/avf/virtchnl.h
> > +++ b/include/linux/avf/virtchnl.h
> > @@ -476,7 +476,6 @@ struct virtchnl_rss_key {
> > u16 vsi_id;
> > u16 key_len;
> > u8 key[1]; /* RSS hash key, packed bytes */
> > - u8 pad[1];
> > };
> >
> > VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
> > @@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
> > u16 vsi_id;
> > u16 lut_entries;
> > u8 lut[1];/* RSS lookup table */
> > - u8 pad[1];
> > };
> >
> > If you use a flexible array member, it should be declared without a size,
> > i.e.
> >
> > u8 key[];
> >
> > Everything else is (trying to) fool the compiler, and leading to undefined
> > behavior, and people (re)adding explicit padding.
>
> This header file is shared across other OSes that use C++ that doesn't support
> flexible arrays. So the structures in this file use an array of size 1 as a
> last
> element to enable variable sized arrays.
I don't think it is accepted practice to have non-Linux-isms in
include/*linux*/avf/virtchnl.h header files. Moreover, using a size
of 1 is counter-intuitive for people used to Linux kernel development,
and may lead to off-by-one errors in calculation of sizes.
If you insist on ignoring the above, this definitely deserves a
comment next to the member's declaration.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH net 1/1] net: phylink: Fix phylink_err() function name error in phylink_major_config
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Mon, 15 Mar 2021 12:33:42 +0800 you wrote:
> if pl->mac_ops->mac_finish() failed, phylink_err should use
> "mac_finish" instead of "mac_prepare".
>
> Fixes: b7ad14c2fe2d4 ("net: phylink: re-implement interface configuration
> with PCS")
> Signed-off-by: Ong Boon Leong
> ---
> drivers/net/phy/phylink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Here is the summary with links:
- [net,1/1] net: phylink: Fix phylink_err() function name error in
phylink_major_config
https://git.kernel.org/netdev/net/c/d82c6c1aaccd
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
[PATCH net 1/1] net: phylink: Fix phylink_err() function name error in phylink_major_config
if pl->mac_ops->mac_finish() failed, phylink_err should use
"mac_finish" instead of "mac_prepare".
Fixes: b7ad14c2fe2d4 ("net: phylink: re-implement interface configuration with
PCS")
Signed-off-by: Ong Boon Leong
---
drivers/net/phy/phylink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 053c92e02cd8..dc2800beacc3 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -476,7 +476,7 @@ static void phylink_major_config(struct phylink *pl, bool
restart,
err = pl->mac_ops->mac_finish(pl->config, pl->cur_link_an_mode,
state->interface);
if (err < 0)
- phylink_err(pl, "mac_prepare failed: %pe\n",
+ phylink_err(pl, "mac_finish failed: %pe\n",
ERR_PTR(err));
}
}
--
2.25.1
Re: [PATCH net 1/1] net: stmmac: Fix VLAN filter delete timeout issue in Intel mGBE SGMII
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 5 Mar 2021 13:49:30 +0800 you wrote: > For Intel mGbE controller, MAC VLAN filter delete operation will time-out > if serdes power-down sequence happened first during driver remove() with > below message. > > [82294.764958] intel-eth-pci :00:1e.4 eth2: stmmac_dvr_remove: removing > driver > [82294.778677] intel-eth-pci :00:1e.4 eth2: Timeout accessing > MAC_VLAN_Tag_Filter > [82294.779997] intel-eth-pci :00:1e.4 eth2: failed to kill vid 0081/0 > [82294.947053] intel-eth-pci :00:1d.2 eth1: stmmac_dvr_remove: removing > driver > [82295.002091] intel-eth-pci :00:1d.1 eth0: stmmac_dvr_remove: removing > driver > > [...] Here is the summary with links: - [net,1/1] net: stmmac: Fix VLAN filter delete timeout issue in Intel mGBE SGMII https://git.kernel.org/netdev/net/c/9a7b3950c7e1 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH net 1/1] stmmac: intel: Fixes clock registration error seen for multiple interfaces
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 5 Mar 2021 14:03:42 +0800 you wrote: > From: Wong Vee Khee > > Issue seen when enumerating multiple Intel mGbE interfaces in EHL. > > [6.898141] intel-eth-pci :00:1d.2: enabling device ( -> 0002) > [6.900971] intel-eth-pci :00:1d.2: Fail to register stmmac-clk > [6.906434] intel-eth-pci :00:1d.2: User ID: 0x51, Synopsys ID: 0x52 > > [...] Here is the summary with links: - [net,1/1] stmmac: intel: Fixes clock registration error seen for multiple interfaces https://git.kernel.org/netdev/net/c/8eb37ab7cc04 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
[PATCH net 1/1] stmmac: intel: Fixes clock registration error seen for multiple interfaces
From: Wong Vee Khee
Issue seen when enumerating multiple Intel mGbE interfaces in EHL.
[6.898141] intel-eth-pci :00:1d.2: enabling device ( -> 0002)
[6.900971] intel-eth-pci :00:1d.2: Fail to register stmmac-clk
[6.906434] intel-eth-pci :00:1d.2: User ID: 0x51, Synopsys ID: 0x52
We fix it by making the clock name to be unique following the format
of stmmac-pci_name(pci_dev) so that we can differentiate the clock for
these Intel mGbE interfaces in EHL platform as follow:
/sys/kernel/debug/clk/stmmac-:00:1d.1
/sys/kernel/debug/clk/stmmac-:00:1d.2
/sys/kernel/debug/clk/stmmac-:00:1e.4
Fixes: 58da0cfa6cf1 ("net: stmmac: create dwmac-intel.c to contain all Intel
platform")
Signed-off-by: Wong Vee Khee
Signed-off-by: Voon Weifeng
Co-developed-by: Ong Boon Leong
Signed-off-by: Ong Boon Leong
---
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index f2896872a86c..0b64f7710d17 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -233,6 +233,7 @@ static void common_default_data(struct plat_stmmacenet_data
*plat)
static int intel_mgbe_common_data(struct pci_dev *pdev,
struct plat_stmmacenet_data *plat)
{
+ char clk_name[20];
int ret;
int i;
@@ -301,8 +302,10 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
plat->eee_usecs_rate = plat->clk_ptp_rate;
/* Set system clock */
+ sprintf(clk_name, "%s-%s", "stmmac", pci_name(pdev));
+
plat->stmmac_clk = clk_register_fixed_rate(>dev,
- "stmmac-clk", NULL, 0,
+ clk_name, NULL, 0,
plat->clk_ptp_rate);
if (IS_ERR(plat->stmmac_clk)) {
--
2.17.0
[PATCH net 1/1] net: stmmac: Fix VLAN filter delete timeout issue in Intel mGBE SGMII
For Intel mGbE controller, MAC VLAN filter delete operation will time-out
if serdes power-down sequence happened first during driver remove() with
below message.
[82294.764958] intel-eth-pci :00:1e.4 eth2: stmmac_dvr_remove: removing
driver
[82294.778677] intel-eth-pci :00:1e.4 eth2: Timeout accessing
MAC_VLAN_Tag_Filter
[82294.779997] intel-eth-pci :00:1e.4 eth2: failed to kill vid 0081/0
[82294.947053] intel-eth-pci :00:1d.2 eth1: stmmac_dvr_remove: removing
driver
[82295.002091] intel-eth-pci :00:1d.1 eth0: stmmac_dvr_remove: removing
driver
Therefore, we delay the serdes power-down to be after unregister_netdev()
which triggers the VLAN filter delete.
Fixes: b9663b7ca6ff ("net: stmmac: Enable SERDES power up/down sequence")
Signed-off-by: Ong Boon Leong
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0eba44e9c1f8..208cae344ffa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5249,13 +5249,16 @@ int stmmac_dvr_remove(struct device *dev)
netdev_info(priv->dev, "%s: removing driver", __func__);
stmmac_stop_all_dma(priv);
+ stmmac_mac_set(priv, priv->ioaddr, false);
+ netif_carrier_off(ndev);
+ unregister_netdev(ndev);
+ /* Serdes power down needs to happen after VLAN filter
+* is deleted that is triggered by unregister_netdev().
+*/
if (priv->plat->serdes_powerdown)
priv->plat->serdes_powerdown(ndev, priv->plat->bsp_priv);
- stmmac_mac_set(priv, priv->ioaddr, false);
- netif_carrier_off(ndev);
- unregister_netdev(ndev);
#ifdef CONFIG_DEBUG_FS
stmmac_exit_fs(ndev);
#endif
--
2.17.0
Re: [PATCH net 1/1] net: stmmac: fix incorrect DMA channel intr enable setting of EQoS v4.10
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Wed, 3 Mar 2021 20:38:40 +0530 you wrote:
> From: Ong Boon Leong
>
> We introduce dwmac410_dma_init_channel() here for both EQoS v4.10 and
> above which use different DMA_CH(n)_Interrupt_Enable bit definitions for
> NIE and AIE.
>
> Fixes: 48863ce5940f ("stmmac: add DMA support for GMAC 4.xx")
> Signed-off-by: Ong Boon Leong
> Signed-off-by: Ramesh Babu B
>
> [...]
Here is the summary with links:
- [net,1/1] net: stmmac: fix incorrect DMA channel intr enable setting of
EQoS v4.10
https://git.kernel.org/netdev/net/c/879c348c35bb
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
RE: [PATCH net 1/1] net: stmmac: fix incorrect DMA channel intr enable setting of EQoS v4.10
> -Original Message-
> From: ramesh.bab...@intel.com
> Sent: 2021年3月3日 23:09
> To: Giuseppe Cavallaro ; Alexandre Torgue
> ; Jose Abreu ; David S .
> Miller ; Jakub Kicinski ; Maxime
> Coquelin
> Cc: net...@vger.kernel.org; linux-st...@st-md-mailman.stormreply.com;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Ong Boon
> Leong ; Voon Wei Feng
> ; Wong Vee Khee ;
> Ramesh Babu B
> Subject: [PATCH net 1/1] net: stmmac: fix incorrect DMA channel intr enable
> setting of EQoS v4.10
>
> From: Ong Boon Leong
>
> We introduce dwmac410_dma_init_channel() here for both EQoS v4.10 and
> above which use different DMA_CH(n)_Interrupt_Enable bit definitions for NIE
> and AIE.
>
> Fixes: 48863ce5940f ("stmmac: add DMA support for GMAC 4.xx")
> Signed-off-by: Ong Boon Leong
> Signed-off-by: Ramesh Babu B
Reviewed-by: Joakim Zhang
Best Regards,
Joakim Zhang
> ---
> .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 19
> ++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> index bb29bfcd62c3..62aa0e95beb7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> @@ -124,6 +124,23 @@ static void dwmac4_dma_init_channel(void __iomem
> *ioaddr,
> ioaddr + DMA_CHAN_INTR_ENA(chan)); }
>
> +static void dwmac410_dma_init_channel(void __iomem *ioaddr,
> + struct stmmac_dma_cfg *dma_cfg, u32 chan)
> {
> + u32 value;
> +
> + /* common channel control register config */
> + value = readl(ioaddr + DMA_CHAN_CONTROL(chan));
> + if (dma_cfg->pblx8)
> + value = value | DMA_BUS_MODE_PBL;
> +
> + writel(value, ioaddr + DMA_CHAN_CONTROL(chan));
> +
> + /* Mask interrupts by writing to CSR7 */
> + writel(DMA_CHAN_INTR_DEFAULT_MASK_4_10,
> +ioaddr + DMA_CHAN_INTR_ENA(chan)); }
> +
> static void dwmac4_dma_init(void __iomem *ioaddr,
> struct stmmac_dma_cfg *dma_cfg, int atds) { @@
> -523,7 +540,7 @@ const struct stmmac_dma_ops dwmac4_dma_ops =
> { const struct stmmac_dma_ops dwmac410_dma_ops = {
> .reset = dwmac4_dma_reset,
> .init = dwmac4_dma_init,
> - .init_chan = dwmac4_dma_init_channel,
> + .init_chan = dwmac410_dma_init_channel,
> .init_rx_chan = dwmac4_dma_init_rx_chan,
> .init_tx_chan = dwmac4_dma_init_tx_chan,
> .axi = dwmac4_dma_axi,
> --
> 2.17.1
[PATCH net 1/1] net: stmmac: fix incorrect DMA channel intr enable setting of EQoS v4.10
From: Ong Boon Leong
We introduce dwmac410_dma_init_channel() here for both EQoS v4.10 and
above which use different DMA_CH(n)_Interrupt_Enable bit definitions for
NIE and AIE.
Fixes: 48863ce5940f ("stmmac: add DMA support for GMAC 4.xx")
Signed-off-by: Ong Boon Leong
Signed-off-by: Ramesh Babu B
---
.../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index bb29bfcd62c3..62aa0e95beb7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -124,6 +124,23 @@ static void dwmac4_dma_init_channel(void __iomem *ioaddr,
ioaddr + DMA_CHAN_INTR_ENA(chan));
}
+static void dwmac410_dma_init_channel(void __iomem *ioaddr,
+ struct stmmac_dma_cfg *dma_cfg, u32 chan)
+{
+ u32 value;
+
+ /* common channel control register config */
+ value = readl(ioaddr + DMA_CHAN_CONTROL(chan));
+ if (dma_cfg->pblx8)
+ value = value | DMA_BUS_MODE_PBL;
+
+ writel(value, ioaddr + DMA_CHAN_CONTROL(chan));
+
+ /* Mask interrupts by writing to CSR7 */
+ writel(DMA_CHAN_INTR_DEFAULT_MASK_4_10,
+ ioaddr + DMA_CHAN_INTR_ENA(chan));
+}
+
static void dwmac4_dma_init(void __iomem *ioaddr,
struct stmmac_dma_cfg *dma_cfg, int atds)
{
@@ -523,7 +540,7 @@ const struct stmmac_dma_ops dwmac4_dma_ops = {
const struct stmmac_dma_ops dwmac410_dma_ops = {
.reset = dwmac4_dma_reset,
.init = dwmac4_dma_init,
- .init_chan = dwmac4_dma_init_channel,
+ .init_chan = dwmac410_dma_init_channel,
.init_rx_chan = dwmac4_dma_init_rx_chan,
.init_tx_chan = dwmac4_dma_init_tx_chan,
.axi = dwmac4_dma_axi,
--
2.17.1
Re: [PATCH net 1/1] stmmac: intel: Fix mdio bus registration issue for TGL-H/ADL-S
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Tue, 2 Mar 2021 16:57:21 +0800 you wrote: > On Intel platforms which consist of two Ethernet Controllers such as > TGL-H and ADL-S, a unique MDIO bus id is required for MDIO bus to be > successful registered: > > [ 13.076133] sysfs: cannot create duplicate filename > '/class/mdio_bus/stmmac-1' > [ 13.083404] CPU: 8 PID: 1898 Comm: systemd-udevd Tainted: G U > 5.11.0-net-next #106 > [ 13.092410] Hardware name: Intel Corporation Alder Lake Client > Platform/AlderLake-S ADP-S DRR4 CRB, BIOS ADLIFSI1.R00.1494.B00.2012031421 > 12/03/2020 > [ 13.105709] Call Trace: > [ 13.108176] dump_stack+0x64/0x7c > [ 13.111553] sysfs_warn_dup+0x56/0x70 > [ 13.115273] sysfs_do_create_link_sd.isra.2+0xbd/0xd0 > [ 13.120371] device_add+0x4df/0x840 > [ 13.123917] ? complete_all+0x2a/0x40 > [ 13.127636] __mdiobus_register+0x98/0x310 [libphy] > [ 13.132572] stmmac_mdio_register+0x1c5/0x3f0 [stmmac] > [ 13.137771] ? stmmac_napi_add+0xa5/0xf0 [stmmac] > [ 13.142493] stmmac_dvr_probe+0x806/0xee0 [stmmac] > [ 13.147341] intel_eth_pci_probe+0x1cb/0x250 [dwmac_intel] > [ 13.152884] pci_device_probe+0xd2/0x150 > [ 13.156897] really_probe+0xf7/0x4d0 > [ 13.160527] driver_probe_device+0x5d/0x140 > [ 13.164761] device_driver_attach+0x4f/0x60 > [ 13.168996] __driver_attach+0xa2/0x140 > [ 13.172891] ? device_driver_attach+0x60/0x60 > [ 13.177300] bus_for_each_dev+0x76/0xc0 > [ 13.181188] bus_add_driver+0x189/0x230 > [ 13.185083] ? 0xc0795000 > [ 13.188446] driver_register+0x5b/0xf0 > [ 13.192249] ? 0xc0795000 > [ 13.195577] do_one_initcall+0x4d/0x210 > [ 13.199467] ? kmem_cache_alloc_trace+0x2ff/0x490 > [ 13.204228] do_init_module+0x5b/0x21c > [ 13.208031] load_module+0x2a0c/0x2de0 > [ 13.211838] ? __do_sys_finit_module+0xb1/0x110 > [ 13.216420] __do_sys_finit_module+0xb1/0x110 > [ 13.220825] do_syscall_64+0x33/0x40 > [ 13.224451] entry_SYSCALL_64_after_hwframe+0x44/0xae > [ 13.229515] RIP: 0033:0x7fc2b1919ccd > [ 13.233113] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 > f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d > 01 f0 ff ff 73 01 c3 48 8b 0d 93 31 0c 00 f7 d8 64 89 01 48 > [ 13.251912] RSP: 002b:7ffcea2e5b98 EFLAGS: 0246 ORIG_RAX: > 0139 > [ 13.259527] RAX: ffda RBX: 560558920f10 RCX: > 7fc2b1919ccd > [ 13.266706] RDX: RSI: 7fc2b1a881e3 RDI: > 0012 > [ 13.273887] RBP: 0002 R08: R09: > > [ 13.281036] R10: 0012 R11: 0246 R12: > 7fc2b1a881e3 > [ 13.288183] R13: R14: R15: > 7ffcea2e5d58 > [ 13.295389] libphy: mii_bus stmmac-1 failed to register > > [...] Here is the summary with links: - [net,1/1] stmmac: intel: Fix mdio bus registration issue for TGL-H/ADL-S https://git.kernel.org/netdev/net/c/fa706dce2f2d You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
[PATCH net 1/1] stmmac: intel: Fix mdio bus registration issue for TGL-H/ADL-S
On Intel platforms which consist of two Ethernet Controllers such as
TGL-H and ADL-S, a unique MDIO bus id is required for MDIO bus to be
successful registered:
[ 13.076133] sysfs: cannot create duplicate filename
'/class/mdio_bus/stmmac-1'
[ 13.083404] CPU: 8 PID: 1898 Comm: systemd-udevd Tainted: G U
5.11.0-net-next #106
[ 13.092410] Hardware name: Intel Corporation Alder Lake Client
Platform/AlderLake-S ADP-S DRR4 CRB, BIOS ADLIFSI1.R00.1494.B00.2012031421
12/03/2020
[ 13.105709] Call Trace:
[ 13.108176] dump_stack+0x64/0x7c
[ 13.111553] sysfs_warn_dup+0x56/0x70
[ 13.115273] sysfs_do_create_link_sd.isra.2+0xbd/0xd0
[ 13.120371] device_add+0x4df/0x840
[ 13.123917] ? complete_all+0x2a/0x40
[ 13.127636] __mdiobus_register+0x98/0x310 [libphy]
[ 13.132572] stmmac_mdio_register+0x1c5/0x3f0 [stmmac]
[ 13.137771] ? stmmac_napi_add+0xa5/0xf0 [stmmac]
[ 13.142493] stmmac_dvr_probe+0x806/0xee0 [stmmac]
[ 13.147341] intel_eth_pci_probe+0x1cb/0x250 [dwmac_intel]
[ 13.152884] pci_device_probe+0xd2/0x150
[ 13.156897] really_probe+0xf7/0x4d0
[ 13.160527] driver_probe_device+0x5d/0x140
[ 13.164761] device_driver_attach+0x4f/0x60
[ 13.168996] __driver_attach+0xa2/0x140
[ 13.172891] ? device_driver_attach+0x60/0x60
[ 13.177300] bus_for_each_dev+0x76/0xc0
[ 13.181188] bus_add_driver+0x189/0x230
[ 13.185083] ? 0xc0795000
[ 13.188446] driver_register+0x5b/0xf0
[ 13.192249] ? 0xc0795000
[ 13.195577] do_one_initcall+0x4d/0x210
[ 13.199467] ? kmem_cache_alloc_trace+0x2ff/0x490
[ 13.204228] do_init_module+0x5b/0x21c
[ 13.208031] load_module+0x2a0c/0x2de0
[ 13.211838] ? __do_sys_finit_module+0xb1/0x110
[ 13.216420] __do_sys_finit_module+0xb1/0x110
[ 13.220825] do_syscall_64+0x33/0x40
[ 13.224451] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 13.229515] RIP: 0033:0x7fc2b1919ccd
[ 13.233113] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89
f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 93 31 0c 00 f7 d8 64 89 01 48
[ 13.251912] RSP: 002b:7ffcea2e5b98 EFLAGS: 0246 ORIG_RAX:
0139
[ 13.259527] RAX: ffda RBX: 560558920f10 RCX: 7fc2b1919ccd
[ 13.266706] RDX: RSI: 7fc2b1a881e3 RDI: 0012
[ 13.273887] RBP: 0002 R08: R09:
[ 13.281036] R10: 0012 R11: 0246 R12: 7fc2b1a881e3
[ 13.288183] R13: R14: R15: 7ffcea2e5d58
[ 13.295389] libphy: mii_bus stmmac-1 failed to register
Fixes: 88af9bd4efbd ("stmmac: intel: Add ADL-S 1Gbps PCI IDs")
Fixes: 8450e23f142f ("stmmac: intel: Add PCI IDs for TGL-H platform")
Signed-off-by: Wong Vee Khee
---
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 54 ++-
1 file changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 74b14d647619..e6eaf378e8e7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -462,8 +462,8 @@ static int tgl_common_data(struct pci_dev *pdev,
return intel_mgbe_common_data(pdev, plat);
}
-static int tgl_sgmii_data(struct pci_dev *pdev,
- struct plat_stmmacenet_data *plat)
+static int tgl_sgmii_phy0_data(struct pci_dev *pdev,
+ struct plat_stmmacenet_data *plat)
{
plat->bus_id = 1;
plat->phy_interface = PHY_INTERFACE_MODE_SGMII;
@@ -472,12 +472,26 @@ static int tgl_sgmii_data(struct pci_dev *pdev,
return tgl_common_data(pdev, plat);
}
-static struct stmmac_pci_info tgl_sgmii1g_info = {
- .setup = tgl_sgmii_data,
+static struct stmmac_pci_info tgl_sgmii1g_phy0_info = {
+ .setup = tgl_sgmii_phy0_data,
};
-static int adls_sgmii_data(struct pci_dev *pdev,
- struct plat_stmmacenet_data *plat)
+static int tgl_sgmii_phy1_data(struct pci_dev *pdev,
+ struct plat_stmmacenet_data *plat)
+{
+ plat->bus_id = 2;
+ plat->phy_interface = PHY_INTERFACE_MODE_SGMII;
+ plat->serdes_powerup = intel_serdes_powerup;
+ plat->serdes_powerdown = intel_serdes_powerdown;
+ return tgl_common_data(pdev, plat);
+}
+
+static struct stmmac_pci_info tgl_sgmii1g_phy1_info = {
+ .setup = tgl_sgmii_phy1_data,
+};
+
+static int adls_sgmii_phy0_data(struct pci_dev *pdev,
+ struct plat_stmmacenet_data *plat)
{
plat->bus_id = 1;
plat->phy_interface = PHY_INTERFACE_MODE_SGMII;
@@ -487,10 +501,24 @@ static int adls_sgmii_data(struct pci_dev *pdev,
return tgl_common_data(pdev, plat);
}
-static struct stmmac_pci_info adls_sgmii1g_info = {
- .setup = adls_sgmii_data,
+static struct
Re: [PATCH net 1/1] net: stmmac: fix CBS idleslope and sendslope calculation
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Thu, 18 Feb 2021 21:40:53 +0800 you wrote: > From: "Song, Yoong Siang" > > When link speed is not 100 Mbps, port transmit rate and speed divider > are set to 8 and 100 respectively. These values are incorrect for > CBS idleslope and sendslope HW values calculation if the link speed is > not 1 Gbps. > > [...] Here is the summary with links: - [net,1/1] net: stmmac: fix CBS idleslope and sendslope calculation https://git.kernel.org/netdev/net/c/24877687b375 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
[PATCH net 1/1] net: stmmac: fix CBS idleslope and sendslope calculation
From: "Song, Yoong Siang"
When link speed is not 100 Mbps, port transmit rate and speed divider
are set to 8 and 100 respectively. These values are incorrect for
CBS idleslope and sendslope HW values calculation if the link speed is
not 1 Gbps.
This patch adds switch statement to set the values of port transmit rate
and speed divider for 10 Gbps, 5 Gbps, 2.5 Gbps, 1 Gbps, and 100 Mbps.
Note that CBS is not supported at 10 Mbps.
Fixes: bc41a6689b30 ("net: stmmac: tc: Remove the speed dependency")
Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
Signed-off-by: Song, Yoong Siang
---
drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 30 +
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 5698554..44bb133 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -316,6 +316,32 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
if (!priv->dma_cap.av)
return -EOPNOTSUPP;
+ /* Port Transmit Rate and Speed Divider */
+ switch (priv->speed) {
+ case SPEED_1:
+ ptr = 32;
+ speed_div = 1000;
+ break;
+ case SPEED_5000:
+ ptr = 32;
+ speed_div = 500;
+ break;
+ case SPEED_2500:
+ ptr = 8;
+ speed_div = 250;
+ break;
+ case SPEED_1000:
+ ptr = 8;
+ speed_div = 100;
+ break;
+ case SPEED_100:
+ ptr = 4;
+ speed_div = 10;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
ret = stmmac_dma_qmode(priv, priv->ioaddr, queue,
MTL_QUEUE_AVB);
@@ -332,10 +358,6 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
}
- /* Port Transmit Rate and Speed Divider */
- ptr = (priv->speed == SPEED_100) ? 4 : 8;
- speed_div = (priv->speed == SPEED_100) ? 10 : 100;
-
/* Final adjustments for HW */
value = div_s64(qopt->idleslope * 1024ll * ptr, speed_div);
priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);
--
2.7.4
Re: [PATCH net 1/2] bridge: mrp: Fix the usage of br_mrp_port_switchdev_set_state
On 06/02/2021 22.47, Horatiu Vultur wrote:
> The function br_mrp_port_switchdev_set_state was called both with MRP
> port state and STP port state, which is an issue because they don't
> match exactly.
>
> Therefore, update the function to be used only with STP port state and
> use the id SWITCHDEV_ATTR_ID_PORT_STP_STATE.
>
> The choice of using STP over MRP is that the drivers already implement
> SWITCHDEV_ATTR_ID_PORT_STP_STATE and already in SW we update the port
> STP state.
>
> Fixes: 9a9f26e8f7ea30 ("bridge: mrp: Connect MRP API with the switchdev API")
> Fixes: fadd409136f0f2 ("bridge: switchdev: mrp: Implement MRP API for
> switchdev")
> Fixes: 2f1a11ae11d222 ("bridge: mrp: Add MRP interface.")
> Reported-by: Rasmus Villemoes
> Signed-off-by: Horatiu Vultur
> ---
Tested-by: Rasmus Villemoes
[PATCH net 1/2] bridge: mrp: Fix the usage of br_mrp_port_switchdev_set_state
The function br_mrp_port_switchdev_set_state was called both with MRP
port state and STP port state, which is an issue because they don't
match exactly.
Therefore, update the function to be used only with STP port state and
use the id SWITCHDEV_ATTR_ID_PORT_STP_STATE.
The choice of using STP over MRP is that the drivers already implement
SWITCHDEV_ATTR_ID_PORT_STP_STATE and already in SW we update the port
STP state.
Fixes: 9a9f26e8f7ea30 ("bridge: mrp: Connect MRP API with the switchdev API")
Fixes: fadd409136f0f2 ("bridge: switchdev: mrp: Implement MRP API for
switchdev")
Fixes: 2f1a11ae11d222 ("bridge: mrp: Add MRP interface.")
Reported-by: Rasmus Villemoes
Signed-off-by: Horatiu Vultur
---
net/bridge/br_mrp.c | 9 ++---
net/bridge/br_mrp_switchdev.c | 7 +++
net/bridge/br_private_mrp.h | 3 +--
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index cec2c4e4561d..5aeae6ad17b3 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -557,19 +557,22 @@ int br_mrp_del(struct net_bridge *br, struct
br_mrp_instance *instance)
int br_mrp_set_port_state(struct net_bridge_port *p,
enum br_mrp_port_state_type state)
{
+ u32 port_state;
+
if (!p || !(p->flags & BR_MRP_AWARE))
return -EINVAL;
spin_lock_bh(>br->lock);
if (state == BR_MRP_PORT_STATE_FORWARDING)
- p->state = BR_STATE_FORWARDING;
+ port_state = BR_STATE_FORWARDING;
else
- p->state = BR_STATE_BLOCKING;
+ port_state = BR_STATE_BLOCKING;
+ p->state = port_state;
spin_unlock_bh(>br->lock);
- br_mrp_port_switchdev_set_state(p, state);
+ br_mrp_port_switchdev_set_state(p, port_state);
return 0;
}
diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
index ed547e03ace1..75a7e8d0a268 100644
--- a/net/bridge/br_mrp_switchdev.c
+++ b/net/bridge/br_mrp_switchdev.c
@@ -169,13 +169,12 @@ int br_mrp_switchdev_send_in_test(struct net_bridge *br,
struct br_mrp *mrp,
return err;
}
-int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
- enum br_mrp_port_state_type state)
+int br_mrp_port_switchdev_set_state(struct net_bridge_port *p, u32 state)
{
struct switchdev_attr attr = {
.orig_dev = p->dev,
- .id = SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
- .u.mrp_port_state = state,
+ .id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
+ .u.stp_state = state,
};
int err;
diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 32a48e5418da..2514954c1431 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -72,8 +72,7 @@ int br_mrp_switchdev_set_ring_state(struct net_bridge *br,
struct br_mrp *mrp,
int br_mrp_switchdev_send_ring_test(struct net_bridge *br, struct br_mrp *mrp,
u32 interval, u8 max_miss, u32 period,
bool monitor);
-int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
- enum br_mrp_port_state_type state);
+int br_mrp_port_switchdev_set_state(struct net_bridge_port *p, u32 state);
int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,
enum br_mrp_port_role_type role);
int br_mrp_switchdev_set_in_role(struct net_bridge *br, struct br_mrp *mrp,
--
2.27.0
Re: [PATCH net 1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Thu, 4 Feb 2021 22:03:16 +0800 you wrote: > From: Mohammad Athari Bin Ismail > > When disable CBS, mode_to_use parameter is not updated even the operation > mode of Tx Queue is changed to Data Centre Bridging (DCB). Therefore, > when tc_setup_cbs() function is called to re-enable CBS, the operation > mode of Tx Queue remains at DCB, which causing CBS fails to work. > > [...] Here is the summary with links: - [net,1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS https://git.kernel.org/netdev/net/c/f317e2ea8c88 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH net 1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS
Song Yoong Siang writes:
> From: Mohammad Athari Bin Ismail
>
> When disable CBS, mode_to_use parameter is not updated even the operation
> mode of Tx Queue is changed to Data Centre Bridging (DCB). Therefore,
> when tc_setup_cbs() function is called to re-enable CBS, the operation
> mode of Tx Queue remains at DCB, which causing CBS fails to work.
>
> This patch updates the value of mode_to_use parameter to MTL_QUEUE_DCB
> after operation mode of Tx Queue is changed to DCB in stmmac_dma_qmode()
> callback function.
>
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Suggested-by: Gomes, Vinicius
Just a nitpick/formality, I would prefer if you used:
Suggested-by: Vinicius Costa Gomes
> Signed-off-by: Mohammad Athari Bin Ismail
> Signed-off-by: Song, Yoong Siang
Patch looks good.
Acked-by: Vinicius Costa Gomes
Cheers,
--
Vinicius
Re: [PATCH net 1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS
Song Yoong Siang wrote:
> From: Mohammad Athari Bin Ismail
>
> When disable CBS, mode_to_use parameter is not updated even the operation
> mode of Tx Queue is changed to Data Centre Bridging (DCB). Therefore,
> when tc_setup_cbs() function is called to re-enable CBS, the operation
> mode of Tx Queue remains at DCB, which causing CBS fails to work.
>
> This patch updates the value of mode_to_use parameter to MTL_QUEUE_DCB
> after operation mode of Tx Queue is changed to DCB in stmmac_dma_qmode()
> callback function.
>
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Suggested-by: Gomes, Vinicius
> Signed-off-by: Mohammad Athari Bin Ismail
> Signed-off-by: Song, Yoong Siang
Reviewed-by: Jesse Brandeburg
[PATCH net 1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS
From: Mohammad Athari Bin Ismail
When disable CBS, mode_to_use parameter is not updated even the operation
mode of Tx Queue is changed to Data Centre Bridging (DCB). Therefore,
when tc_setup_cbs() function is called to re-enable CBS, the operation
mode of Tx Queue remains at DCB, which causing CBS fails to work.
This patch updates the value of mode_to_use parameter to MTL_QUEUE_DCB
after operation mode of Tx Queue is changed to DCB in stmmac_dma_qmode()
callback function.
Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
Suggested-by: Gomes, Vinicius
Signed-off-by: Mohammad Athari Bin Ismail
Signed-off-by: Song, Yoong Siang
---
drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 8ed3b2c..5698554 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -324,7 +324,12 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
} else if (!qopt->enable) {
- return stmmac_dma_qmode(priv, priv->ioaddr, queue,
MTL_QUEUE_DCB);
+ ret = stmmac_dma_qmode(priv, priv->ioaddr, queue,
+ MTL_QUEUE_DCB);
+ if (ret)
+ return ret;
+
+ priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
}
/* Port Transmit Rate and Speed Divider */
--
2.7.4
Re: [PATCH net 1/4] ipv6: silence compilation warning for non-IPV6 builds
On Tue, 2 Feb 2021 20:55:28 +0200 Leon Romanovsky wrote:
> On Tue, Feb 02, 2021 at 08:29:09AM -0800, Jakub Kicinski wrote:
> > On Tue, 2 Feb 2021 15:55:41 +0200 Leon Romanovsky wrote:
> > > From: Leon Romanovsky
> > >
> > > The W=1 compilation of allmodconfig generates the following warning:
> > >
> > > net/ipv6/icmp.c:448:6: warning: no previous prototype for 'icmp6_send'
> > > [-Wmissing-prototypes]
> > > 448 | void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
> > > | ^~
> > >
> > > In such configuration, the icmp6_send() is not used outside of icmp.c, so
> > > close
> > > its EXPORT_SYMBOL and add "static" word to limit the scope.
> > >
> > > Fixes: cc7a21b6fbd9 ("ipv6: icmp6: avoid indirect call for icmpv6_send()")
> > > Signed-off-by: Leon Romanovsky
> >
> > That's a little much ifdefinery, why not move the declaration from
> > under the ifdef in the header instead?
>
> We will find ourselves with exported but not used function, it will
> increase symbol file, not big deal but not nice, either.
For those all so common builds where IPv6 is a module :)
But I don't feel strongly, up to you.
> > If you repost please target net-next, admittedly these fixes are pretty
> > "obviously correct" but they are not urgent either.
>
> I'll do.
Thanks!
Re: [PATCH net 1/4] ipv6: silence compilation warning for non-IPV6 builds
On Tue, Feb 02, 2021 at 08:29:09AM -0800, Jakub Kicinski wrote:
> On Tue, 2 Feb 2021 15:55:41 +0200 Leon Romanovsky wrote:
> > From: Leon Romanovsky
> >
> > The W=1 compilation of allmodconfig generates the following warning:
> >
> > net/ipv6/icmp.c:448:6: warning: no previous prototype for 'icmp6_send'
> > [-Wmissing-prototypes]
> > 448 | void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
> > | ^~
> >
> > In such configuration, the icmp6_send() is not used outside of icmp.c, so
> > close
> > its EXPORT_SYMBOL and add "static" word to limit the scope.
> >
> > Fixes: cc7a21b6fbd9 ("ipv6: icmp6: avoid indirect call for icmpv6_send()")
> > Signed-off-by: Leon Romanovsky
>
> That's a little much ifdefinery, why not move the declaration from
> under the ifdef in the header instead?
We will find ourselves with exported but not used function, it will
increase symbol file, not big deal but not nice, either.
>
> If you repost please target net-next, admittedly these fixes are pretty
> "obviously correct" but they are not urgent either.
I'll do.
Thanks
Re: [PATCH net 1/4] ipv6: silence compilation warning for non-IPV6 builds
On Tue, 2 Feb 2021 15:55:41 +0200 Leon Romanovsky wrote:
> From: Leon Romanovsky
>
> The W=1 compilation of allmodconfig generates the following warning:
>
> net/ipv6/icmp.c:448:6: warning: no previous prototype for 'icmp6_send'
> [-Wmissing-prototypes]
> 448 | void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
> | ^~
>
> In such configuration, the icmp6_send() is not used outside of icmp.c, so
> close
> its EXPORT_SYMBOL and add "static" word to limit the scope.
>
> Fixes: cc7a21b6fbd9 ("ipv6: icmp6: avoid indirect call for icmpv6_send()")
> Signed-off-by: Leon Romanovsky
That's a little much ifdefinery, why not move the declaration from
under the ifdef in the header instead?
If you repost please target net-next, admittedly these fixes are pretty
"obviously correct" but they are not urgent either.
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index f3d05866692e..5d4232b492dc 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -445,6 +445,9 @@ static int icmp6_iif(const struct sk_buff *skb)
> /*
> * Send an ICMP message in response to a packet in error
> */
> +#if !IS_BUILTIN(CONFIG_IPV6)
> +static
> +#endif
> void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
> const struct in6_addr *force_saddr)
> {
> @@ -634,7 +637,10 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code,
> __u32 info,
> out_bh_enable:
> local_bh_enable();
> }
> +
> +#if IS_BUILTIN(CONFIG_IPV6)
> EXPORT_SYMBOL(icmp6_send);
> +#endif
>
> /* Slightly more convenient version of icmp6_send.
> */
> --
> 2.29.2
>
[PATCH net 1/4] ipv6: silence compilation warning for non-IPV6 builds
From: Leon Romanovsky
The W=1 compilation of allmodconfig generates the following warning:
net/ipv6/icmp.c:448:6: warning: no previous prototype for 'icmp6_send'
[-Wmissing-prototypes]
448 | void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
| ^~
In such configuration, the icmp6_send() is not used outside of icmp.c, so close
its EXPORT_SYMBOL and add "static" word to limit the scope.
Fixes: cc7a21b6fbd9 ("ipv6: icmp6: avoid indirect call for icmpv6_send()")
Signed-off-by: Leon Romanovsky
---
net/ipv6/icmp.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index f3d05866692e..5d4232b492dc 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -445,6 +445,9 @@ static int icmp6_iif(const struct sk_buff *skb)
/*
* Send an ICMP message in response to a packet in error
*/
+#if !IS_BUILTIN(CONFIG_IPV6)
+static
+#endif
void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
const struct in6_addr *force_saddr)
{
@@ -634,7 +637,10 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code,
__u32 info,
out_bh_enable:
local_bh_enable();
}
+
+#if IS_BUILTIN(CONFIG_IPV6)
EXPORT_SYMBOL(icmp6_send);
+#endif
/* Slightly more convenient version of icmp6_send.
*/
--
2.29.2
Re: [PATCH net 1/4] net: ipa: add a missing __iomem attribute
On Mon, Feb 1, 2021 at 3:29 PM Alex Elder wrote:
>
> The virt local variable in gsi_channel_state() does not have an
> __iomem attribute but should. Fix this.
>
> Signed-off-by: Alex Elder
> ---
> drivers/net/ipa/gsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
> index 14d9a791924bf..e2e77f09077a9 100644
> --- a/drivers/net/ipa/gsi.c
> +++ b/drivers/net/ipa/gsi.c
> @@ -440,7 +440,7 @@ static void gsi_evt_ring_de_alloc_command(struct gsi
> *gsi, u32 evt_ring_id)
> static enum gsi_channel_state gsi_channel_state(struct gsi_channel *channel)
> {
> u32 channel_id = gsi_channel_id(channel);
> - void *virt = channel->gsi->virt;
> + void __iomem *virt = channel->gsi->virt;
> u32 val;
>
> val = ioread32(virt + GSI_CH_C_CNTXT_0_OFFSET(channel_id));
> --
> 2.27.0
>
Seems pretty straightforward to me, ioread32 expects an
__iomem-annotated pointer.
Reviewed-by: Amy Parker
[PATCH net 1/4] net: ipa: add a missing __iomem attribute
The virt local variable in gsi_channel_state() does not have an
__iomem attribute but should. Fix this.
Signed-off-by: Alex Elder
---
drivers/net/ipa/gsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 14d9a791924bf..e2e77f09077a9 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -440,7 +440,7 @@ static void gsi_evt_ring_de_alloc_command(struct gsi *gsi,
u32 evt_ring_id)
static enum gsi_channel_state gsi_channel_state(struct gsi_channel *channel)
{
u32 channel_id = gsi_channel_id(channel);
- void *virt = channel->gsi->virt;
+ void __iomem *virt = channel->gsi->virt;
u32 val;
val = ioread32(virt + GSI_CH_C_CNTXT_0_OFFSET(channel_id));
--
2.27.0
RE: [EXTERNAL] Re: [PATCH net 1/1] net: fec: Fix temporary RMII clock reset on link up
> drivers/net/ethernet/freescale/fec_main.c: In function ‘fec_restart’:
> drivers/net/ethernet/freescale/fec_main.c:958:46: warning: suggest
> parentheses around ‘&&’ within ‘||’ [-Wparentheses]
> 958 | (fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link) {
> | ^~~~
Thank you very much for taking the time to review; I'm sorry, I should have
caught the warning, I will fix this asap.
Best regards,
Laurent
-
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la
Longeraie 7, 1110, Morges, Switzerland
-
Re: [PATCH net 1/1] net: fec: Fix temporary RMII clock reset on link up
On Fri, 22 Jan 2021 16:13:47 +0100 Laurent Badel wrote:
> fec_restart() does a hard reset of the MAC module when the link status
> changes to up. This temporarily resets the R_CNTRL register which controls
> the MII mode of the ENET_OUT clock. In the case of RMII, the clock
> frequency momentarily drops from 50MHz to 25MHz until the register is
> reconfigured. Some link partners do not tolerate this glitch and
> invalidate the link causing failure to establish a stable link when using
> PHY polling mode. Since as per IEEE802.11 the criteria for link validity
> are PHY-specific, what the partner should tolerate cannot be assumed, so
> avoid resetting the MII clock by using software reset instead of hardware
> reset when the link is up. This is generally relevant only if the SoC
> provides the clock to an external PHY and the PHY is configured for RMII.
> static const struct fec_devinfo fec_imx6q_info = {
> @@ -953,7 +954,8 @@ fec_restart(struct net_device *ndev)
>* For i.MX6SX SOC, enet use AXI bus, we use disable MAC
>* instead of reset MAC itself.
>*/
> - if (fep->quirks & FEC_QUIRK_HAS_AVB) {
> + if (fep->quirks & FEC_QUIRK_HAS_AVB ||
> + (fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link) {
> writel(0, fep->hwp + FEC_ECNTRL);
> } else {
> writel(1, fep->hwp + FEC_ECNTRL);
drivers/net/ethernet/freescale/fec_main.c: In function ‘fec_restart’:
drivers/net/ethernet/freescale/fec_main.c:958:46: warning: suggest parentheses
around ‘&&’ within ‘||’ [-Wparentheses]
958 | (fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link) {
| ^~~~
[PATCH net 1/1] net: fec: Fix temporary RMII clock reset on link up
fec_restart() does a hard reset of the MAC module when the link status
changes to up. This temporarily resets the R_CNTRL register which controls
the MII mode of the ENET_OUT clock. In the case of RMII, the clock
frequency momentarily drops from 50MHz to 25MHz until the register is
reconfigured. Some link partners do not tolerate this glitch and
invalidate the link causing failure to establish a stable link when using
PHY polling mode. Since as per IEEE802.11 the criteria for link validity
are PHY-specific, what the partner should tolerate cannot be assumed, so
avoid resetting the MII clock by using software reset instead of hardware
reset when the link is up. This is generally relevant only if the SoC
provides the clock to an external PHY and the PHY is configured for RMII.
Signed-off-by: Laurent Badel
---
drivers/net/ethernet/freescale/fec.h | 5 +
drivers/net/ethernet/freescale/fec_main.c | 6 --
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h
b/drivers/net/ethernet/freescale/fec.h
index c527f4ee1d3a..0602d5d5d2ee 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -462,6 +462,11 @@ struct bufdesc_ex {
*/
#define FEC_QUIRK_CLEAR_SETUP_MII (1 << 17)
+/* Some link partners do not tolerate the momentary reset of the REF_CLK
+ * frequency when the RNCTL register is cleared by hardware reset.
+ */
+#define FEC_QUIRK_NO_HARD_RESET(1 << 18)
+
struct bufdesc_prop {
int qid;
/* Address of Rx and Tx buffers */
diff --git a/drivers/net/ethernet/freescale/fec_main.c
b/drivers/net/ethernet/freescale/fec_main.c
index 04f24c66cf36..280ccea4327f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -100,7 +100,8 @@ static const struct fec_devinfo fec_imx27_info = {
static const struct fec_devinfo fec_imx28_info = {
.quirks = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME |
FEC_QUIRK_SINGLE_MDIO | FEC_QUIRK_HAS_RACC |
- FEC_QUIRK_HAS_FRREG | FEC_QUIRK_CLEAR_SETUP_MII,
+ FEC_QUIRK_HAS_FRREG | FEC_QUIRK_CLEAR_SETUP_MII |
+ FEC_QUIRK_NO_HARD_RESET,
};
static const struct fec_devinfo fec_imx6q_info = {
@@ -953,7 +954,8 @@ fec_restart(struct net_device *ndev)
* For i.MX6SX SOC, enet use AXI bus, we use disable MAC
* instead of reset MAC itself.
*/
- if (fep->quirks & FEC_QUIRK_HAS_AVB) {
+ if (fep->quirks & FEC_QUIRK_HAS_AVB ||
+ (fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link) {
writel(0, fep->hwp + FEC_ECNTRL);
} else {
writel(1, fep->hwp + FEC_ECNTRL);
--
2.17.1
-
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la
Longeraie 7, 1110, Morges, Switzerland
-
[PATCH net 1/1] net: phy: Reconfigure PHY interrupt in mdio_bus_phy_restore()
Some PHY (e.g. SMSC LAN87xx) clear their interrupt mask on software
reset. This breaks the ethernet interface on resuming from hibernation,
if the PHY is running in interrupt mode, so reconfigure interrupts
after the software reset in mdio_bus_phy_restore().
Signed-off-by: Laurent Badel
---
drivers/net/phy/phy_device.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 80c2e646c093..5070eed55447 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -324,6 +324,15 @@ static int mdio_bus_phy_restore(struct device *dev)
if (ret < 0)
return ret;
+ if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
+ {
+ /* Some PHYs (e.g. SMSC LAN8720) clear their
+* interrupt mask on software reset.
+*/
+ phy_free_interrupt(phydev);
+ phy_request_interrupt(phydev);
+ }
+
if (phydev->attached_dev && phydev->adjust_link)
phy_start_machine(phydev);
--
2.17.1
-
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la
Longeraie 7, 1110, Morges, Switzerland
-
Re: [RFC PATCH net 1/2] net: introduce CAN specific pointer in the struct net_device
On Fri, Jan 15, 2021 at 04:07:23PM -0800, Jakub Kicinski wrote:
> On Fri, 15 Jan 2021 15:30:35 +0100 Oleksij Rempel wrote:
> > Since 20dd3850bcf8 ("can: Speed up CAN frame receiption by using
> > ml_priv") the CAN framework uses per device specific data in the AF_CAN
> > protocol. For this purpose the struct net_device->ml_priv is used. Later
> > the ml_priv usage in CAN was extended for other users, one of them being
> > CAN_J1939.
> >
> > Later in the kernel ml_priv was converted to an union, used by other
> > drivers. E.g. the tun driver started storing it's stats pointer.
> >
> > Since tun devices can claim to be a CAN device, CAN specific protocols
> > will wrongly interpret this pointer, which will cause system crashes.
> > Mostly this issue is visible in the CAN_J1939 stack.
> >
> > To fix this issue, we request a dedicated CAN pointer within the
> > net_device struct.
>
> No strong objection, others already added their pointers, but
> I wonder if we can't save those couple of bytes by adding a
> ml_priv_type, to check instead of dev->type? And leave it 0
> for drivers using stats?
Sounds good.
If we want to save even more bytes, it might be possible, to move the
wireless and wpan pointers to ml_priv.
struct wireless_dev *ieee80211_ptr;
struct wpan_dev *ieee802154_ptr;
> That way other device types which are limited by all being
> ARPHDR_ETHER can start using ml_priv as well?
>
> > +#if IS_ENABLED(CONFIG_CAN)
> > + struct can_ml_priv *can;
> > +#endif
>
> Perhaps put it next to all the _ptr fields?
Makes sense. Oleksij will resping the series.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
signature.asc
Description: PGP signature
Re: [PATCH net 1/4] net: dsa: mv88e6xxx: Remove bogus Kconfig dependency.
On 1/20/2021 10:06 PM, Richard Cochran wrote:
The mv88e6xxx is a DSA driver, and it implements DSA style time
stamping of PTP frames. It has no need of the expensive option to
enable PHY time stamping. Remove the bogus dependency.
Signed-off-by: Richard Cochran
Fixes: 2fa8d3af4bad ("net: dsa: mv88e6xxx: expose switch time as a PTP hardware
clock")
Ah, I must have thought I needed skb_defer_rx_timestamp in mv88e6xxx,
but instead DSA gained its own timestamp path so mv88e6xxx turned out
to not need that at all, and I never went back and dropped the Kconfig
dependency. Whoops. Yup, I don't think we don't need it here.
Acked-by: Brandon Streiff
[PATCH net 1/4] net: dsa: mv88e6xxx: Remove bogus Kconfig dependency.
The mv88e6xxx is a DSA driver, and it implements DSA style time
stamping of PTP frames. It has no need of the expensive option to
enable PHY time stamping. Remove the bogus dependency.
Signed-off-by: Richard Cochran
Fixes: 2fa8d3af4bad ("net: dsa: mv88e6xxx: expose switch time as a PTP hardware
clock")
---
drivers/net/dsa/mv88e6xxx/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig
b/drivers/net/dsa/mv88e6xxx/Kconfig
index 51185e4d7d15..b17540926c11 100644
--- a/drivers/net/dsa/mv88e6xxx/Kconfig
+++ b/drivers/net/dsa/mv88e6xxx/Kconfig
@@ -25,7 +25,6 @@ config NET_DSA_MV88E6XXX_PTP
default n
depends on NET_DSA_MV88E6XXX_GLOBAL2
depends on PTP_1588_CLOCK
- imply NETWORK_PHY_TIMESTAMPING
help
Say Y to enable PTP hardware timestamping on Marvell 88E6xxx switch
chips that support it.
--
2.20.1
Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets
On Tue, 19 Jan 2021 16:42:14 +0100 Rasmus Villemoes wrote:
> On 28/12/2020 23.24, Jakub Kicinski wrote:
> > On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:
> >> Wireshark says that the MRP test packets cannot be decoded - and the
> >> reason for that is that there's a two-byte hole filled with garbage
> >> between the "transitions" and "timestamp" members.
> >>
> >> So Wireshark decodes the two garbage bytes and the top two bytes of
> >> the timestamp written by the kernel as the timestamp value (which thus
> >> fluctuates wildly), and interprets the lower two bytes of the
> >> timestamp as a new (type, length) pair, which is of course broken.
> >>
> >> While my copy of the MRP standard is still under way [*], I cannot
> >> imagine the standard specifying a two-byte hole here, and whoever
> >> wrote the Wireshark decoding code seems to agree with that.
> >>
> >> The struct definitions live under include/uapi/, but they are not
> >> really part of any kernel<->userspace API/ABI, so fixing the
> >> definitions by adding the packed attribute should not cause any
> >> compatibility issues.
> >>
> >> The remaining on-the-wire packet formats likely also don't contain
> >> holes, but pahole and manual inspection says the current definitions
> >> suffice. So adding the packed attribute to those is not strictly
> >> needed, but might be done for good measure.
> >>
> >> [*] I will never understand how something hidden behind a +1000$
> >> paywall can be called a standard.
> >>
> >> Signed-off-by: Rasmus Villemoes
> >> ---
> >> include/uapi/linux/mrp_bridge.h | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/mrp_bridge.h
> >> b/include/uapi/linux/mrp_bridge.h
> >> index 6aeb13ef0b1e..d1d0cf65916d 100644
> >> --- a/include/uapi/linux/mrp_bridge.h
> >> +++ b/include/uapi/linux/mrp_bridge.h
> >> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> >>__be16 state;
> >>__be16 transitions;
> >>__be32 timestamp;
> >> -};
> >> +} __attribute__((__packed__));
> >>
> >> struct br_mrp_ring_topo_hdr {
> >>__be16 prio;
> >> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
> >>__be16 state;
> >>__be16 transitions;
> >>__be32 timestamp;
> >> -};
> >> +} __attribute__((__packed__));
> >>
> >> struct br_mrp_in_topo_hdr {
> >>__u8 sa[ETH_ALEN];
> >
> > Can we use this opportunity to move the definitions of these structures
> > out of the uAPI to a normal kernel header?
>
> Jakub, can we apply this patch to net, then later move the definitions
> out of uapi (and deleting the unused ones in the process)?
This has been lost in the patchwork archives already, we'll need a
fresh posting anyway. Why not do the move as a part of the same series?
Doesn't seems like much work, unless I'm missing something..
Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets
On 28/12/2020 23.24, Jakub Kicinski wrote:
> On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:
>> Wireshark says that the MRP test packets cannot be decoded - and the
>> reason for that is that there's a two-byte hole filled with garbage
>> between the "transitions" and "timestamp" members.
>>
>> So Wireshark decodes the two garbage bytes and the top two bytes of
>> the timestamp written by the kernel as the timestamp value (which thus
>> fluctuates wildly), and interprets the lower two bytes of the
>> timestamp as a new (type, length) pair, which is of course broken.
>>
>> While my copy of the MRP standard is still under way [*], I cannot
>> imagine the standard specifying a two-byte hole here, and whoever
>> wrote the Wireshark decoding code seems to agree with that.
>>
>> The struct definitions live under include/uapi/, but they are not
>> really part of any kernel<->userspace API/ABI, so fixing the
>> definitions by adding the packed attribute should not cause any
>> compatibility issues.
>>
>> The remaining on-the-wire packet formats likely also don't contain
>> holes, but pahole and manual inspection says the current definitions
>> suffice. So adding the packed attribute to those is not strictly
>> needed, but might be done for good measure.
>>
>> [*] I will never understand how something hidden behind a +1000$
>> paywall can be called a standard.
>>
>> Signed-off-by: Rasmus Villemoes
>> ---
>> include/uapi/linux/mrp_bridge.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/mrp_bridge.h
>> b/include/uapi/linux/mrp_bridge.h
>> index 6aeb13ef0b1e..d1d0cf65916d 100644
>> --- a/include/uapi/linux/mrp_bridge.h
>> +++ b/include/uapi/linux/mrp_bridge.h
>> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
>> __be16 state;
>> __be16 transitions;
>> __be32 timestamp;
>> -};
>> +} __attribute__((__packed__));
>>
>> struct br_mrp_ring_topo_hdr {
>> __be16 prio;
>> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
>> __be16 state;
>> __be16 transitions;
>> __be32 timestamp;
>> -};
>> +} __attribute__((__packed__));
>>
>> struct br_mrp_in_topo_hdr {
>> __u8 sa[ETH_ALEN];
>
> Can we use this opportunity to move the definitions of these structures
> out of the uAPI to a normal kernel header?
>
Jakub, can we apply this patch to net, then later move the definitions
out of uapi (and deleting the unused ones in the process)?
Thanks,
Rasmus
--
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
rasmus.villem...@prevas.dk
www.prevas.dk
Re: [PATCH net 1/2] net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext
On Sat, Jan 16, 2021 at 03:39, Rasmus Villemoes wrote: > mv88e6xxx_port_vlan_join checks whether the VTU already contains an > entry for the given vid (via mv88e6xxx_vtu_getnext), and if so, merely > changes the relevant .member[] element and loads the updated entry > into the VTU. > > However, at least for the mv88e6250, the on-stack struct > mv88e6xxx_vtu_entry vlan never has its .state[] array explicitly > initialized, neither in mv88e6xxx_port_vlan_join() nor inside the > getnext implementation. So the new entry has random garbage for the > STU bits, breaking VLAN filtering. > > When the VTU entry is initially created, those bits are all zero, and > we should make sure to keep them that way when the entry is updated. > > Fixes: 92307069a96c (net: dsa: mv88e6xxx: Avoid VTU corruption on 6097) > Signed-off-by: Rasmus Villemoes Reviewed-by: Tobias Waldekranz Tested-by: Tobias Waldekranz
Re: [PATCH net 1/2] ipv6: create multicast route with RTPROT_KERNEL
On Sat, Jan 16, 2021 at 5:36 AM David Ahern wrote: > > On 1/15/21 11:42 AM, Matteo Croce wrote: > > From: Matteo Croce > > > > The ff00::/8 multicast route is created without specifying the fc_protocol > > field, so the default RTPROT_BOOT value is used: > > > > $ ip -6 -d route > > unicast ::1 dev lo proto kernel scope global metric 256 pref medium > > unicast fe80::/64 dev eth0 proto kernel scope global metric 256 pref > > medium > > unicast ff00::/8 dev eth0 proto boot scope global metric 256 pref medium > > > > As the documentation says, this value identifies routes installed during > > boot, but the route is created when interface is set up. > > Change the value to RTPROT_KERNEL which is a better value. > > > > Signed-off-by: Matteo Croce > > --- > > net/ipv6/addrconf.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > > index eff2cacd5209..19bf6822911c 100644 > > --- a/net/ipv6/addrconf.c > > +++ b/net/ipv6/addrconf.c > > @@ -2469,6 +2469,7 @@ static void addrconf_add_mroute(struct net_device > > *dev) > > .fc_flags = RTF_UP, > > .fc_type = RTN_UNICAST, > > .fc_nlinfo.nl_net = dev_net(dev), > > + .fc_protocol = RTPROT_KERNEL, > > }; > > > > ipv6_addr_set(_dst, htonl(0xFF00), 0, 0, 0); > > > > > What's the motivation for changing this? ie., what s/w cares that it is > kernel vs boot? A practical example: systemd-networkd explicitly ignores routes with kernel flag[1]. Having a different flag leads the daemon to remove the route sooner or later. [1] https://github.com/systemd/systemd/blob/0b81225e5791f660506f7db0ab88078cf296b771/src/network/networkd-routing-policy-rule.c#L675-L677 -- per aspera ad upstream
Re: [PATCH net 1/2] net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext
On 1/15/2021 6:39 PM, Rasmus Villemoes wrote: > mv88e6xxx_port_vlan_join checks whether the VTU already contains an > entry for the given vid (via mv88e6xxx_vtu_getnext), and if so, merely > changes the relevant .member[] element and loads the updated entry > into the VTU. > > However, at least for the mv88e6250, the on-stack struct > mv88e6xxx_vtu_entry vlan never has its .state[] array explicitly > initialized, neither in mv88e6xxx_port_vlan_join() nor inside the > getnext implementation. So the new entry has random garbage for the > STU bits, breaking VLAN filtering. > > When the VTU entry is initially created, those bits are all zero, and > we should make sure to keep them that way when the entry is updated. > > Fixes: 92307069a96c (net: dsa: mv88e6xxx: Avoid VTU corruption on 6097) > Signed-off-by: Rasmus Villemoes Reviewed-by: Florian Fainelli -- Florian
Re: [PATCH net 1/2] ipv6: create multicast route with RTPROT_KERNEL
On 1/15/21 11:42 AM, Matteo Croce wrote: > From: Matteo Croce > > The ff00::/8 multicast route is created without specifying the fc_protocol > field, so the default RTPROT_BOOT value is used: > > $ ip -6 -d route > unicast ::1 dev lo proto kernel scope global metric 256 pref medium > unicast fe80::/64 dev eth0 proto kernel scope global metric 256 pref medium > unicast ff00::/8 dev eth0 proto boot scope global metric 256 pref medium > > As the documentation says, this value identifies routes installed during > boot, but the route is created when interface is set up. > Change the value to RTPROT_KERNEL which is a better value. > > Signed-off-by: Matteo Croce > --- > net/ipv6/addrconf.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index eff2cacd5209..19bf6822911c 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -2469,6 +2469,7 @@ static void addrconf_add_mroute(struct net_device *dev) > .fc_flags = RTF_UP, > .fc_type = RTN_UNICAST, > .fc_nlinfo.nl_net = dev_net(dev), > + .fc_protocol = RTPROT_KERNEL, > }; > > ipv6_addr_set(_dst, htonl(0xFF00), 0, 0, 0); > What's the motivation for changing this? ie., what s/w cares that it is kernel vs boot?
[PATCH net 1/2] net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext
mv88e6xxx_port_vlan_join checks whether the VTU already contains an entry for the given vid (via mv88e6xxx_vtu_getnext), and if so, merely changes the relevant .member[] element and loads the updated entry into the VTU. However, at least for the mv88e6250, the on-stack struct mv88e6xxx_vtu_entry vlan never has its .state[] array explicitly initialized, neither in mv88e6xxx_port_vlan_join() nor inside the getnext implementation. So the new entry has random garbage for the STU bits, breaking VLAN filtering. When the VTU entry is initially created, those bits are all zero, and we should make sure to keep them that way when the entry is updated. Fixes: 92307069a96c (net: dsa: mv88e6xxx: Avoid VTU corruption on 6097) Signed-off-by: Rasmus Villemoes --- drivers/net/dsa/mv88e6xxx/global1_vtu.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c index 66ddf67b8737..7b96396be609 100644 --- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c +++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c @@ -351,6 +351,10 @@ int mv88e6250_g1_vtu_getnext(struct mv88e6xxx_chip *chip, if (err) return err; + err = mv88e6185_g1_stu_data_read(chip, entry); + if (err) + return err; + /* VTU DBNum[3:0] are located in VTU Operation 3:0 * VTU DBNum[5:4] are located in VTU Operation 9:8 */ -- 2.23.0
Re: [RFC PATCH net 1/2] net: introduce CAN specific pointer in the struct net_device
On Fri, 15 Jan 2021 15:30:35 +0100 Oleksij Rempel wrote:
> Since 20dd3850bcf8 ("can: Speed up CAN frame receiption by using
> ml_priv") the CAN framework uses per device specific data in the AF_CAN
> protocol. For this purpose the struct net_device->ml_priv is used. Later
> the ml_priv usage in CAN was extended for other users, one of them being
> CAN_J1939.
>
> Later in the kernel ml_priv was converted to an union, used by other
> drivers. E.g. the tun driver started storing it's stats pointer.
>
> Since tun devices can claim to be a CAN device, CAN specific protocols
> will wrongly interpret this pointer, which will cause system crashes.
> Mostly this issue is visible in the CAN_J1939 stack.
>
> To fix this issue, we request a dedicated CAN pointer within the
> net_device struct.
No strong objection, others already added their pointers, but
I wonder if we can't save those couple of bytes by adding a
ml_priv_type, to check instead of dev->type? And leave it 0
for drivers using stats?
That way other device types which are limited by all being
ARPHDR_ETHER can start using ml_priv as well?
> +#if IS_ENABLED(CONFIG_CAN)
> + struct can_ml_priv *can;
> +#endif
Perhaps put it next to all the _ptr fields?
[PATCH net 1/2] ipv6: create multicast route with RTPROT_KERNEL
From: Matteo Croce The ff00::/8 multicast route is created without specifying the fc_protocol field, so the default RTPROT_BOOT value is used: $ ip -6 -d route unicast ::1 dev lo proto kernel scope global metric 256 pref medium unicast fe80::/64 dev eth0 proto kernel scope global metric 256 pref medium unicast ff00::/8 dev eth0 proto boot scope global metric 256 pref medium As the documentation says, this value identifies routes installed during boot, but the route is created when interface is set up. Change the value to RTPROT_KERNEL which is a better value. Signed-off-by: Matteo Croce --- net/ipv6/addrconf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index eff2cacd5209..19bf6822911c 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -2469,6 +2469,7 @@ static void addrconf_add_mroute(struct net_device *dev) .fc_flags = RTF_UP, .fc_type = RTN_UNICAST, .fc_nlinfo.nl_net = dev_net(dev), + .fc_protocol = RTPROT_KERNEL, }; ipv6_addr_set(_dst, htonl(0xFF00), 0, 0, 0); -- 2.29.2
[RFC PATCH net 1/2] net: introduce CAN specific pointer in the struct net_device
Since 20dd3850bcf8 ("can: Speed up CAN frame receiption by using
ml_priv") the CAN framework uses per device specific data in the AF_CAN
protocol. For this purpose the struct net_device->ml_priv is used. Later
the ml_priv usage in CAN was extended for other users, one of them being
CAN_J1939.
Later in the kernel ml_priv was converted to an union, used by other
drivers. E.g. the tun driver started storing it's stats pointer.
Since tun devices can claim to be a CAN device, CAN specific protocols
will wrongly interpret this pointer, which will cause system crashes.
Mostly this issue is visible in the CAN_J1939 stack.
To fix this issue, we request a dedicated CAN pointer within the
net_device struct.
Reported-by: syzbot+5138c4dd15a0401be...@syzkaller.appspotmail.com
Fixes: 20dd3850bcf8 ("can: Speed up CAN frame receiption by using ml_priv")
Fixes: ffd956eef69b ("can: introduce CAN midlayer private and allocate it
automatically")
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Fixes: 497a5757ce4e ("tun: switch to net core provided statistics counters")
Signed-off-by: Oleksij Rempel
---
drivers/net/can/dev/dev.c | 2 +-
drivers/net/can/slcan.c | 2 +-
drivers/net/can/vcan.c| 2 +-
drivers/net/can/vxcan.c | 2 +-
include/linux/netdevice.h | 4
net/can/af_can.c | 4 ++--
net/can/j1939/main.c | 4 ++--
net/can/j1939/socket.c| 2 +-
net/can/proc.c| 13 +++--
9 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index f580f0ac6497..0e5265cb635f 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -269,7 +269,7 @@ struct net_device *alloc_candev_mqs(int sizeof_priv,
unsigned int echo_skb_max,
priv = netdev_priv(dev);
priv->dev = dev;
- dev->ml_priv = (void *)priv + ALIGN(sizeof_priv, NETDEV_ALIGN);
+ dev->can = (void *)priv + ALIGN(sizeof_priv, NETDEV_ALIGN);
if (echo_skb_max) {
priv->echo_skb_max = echo_skb_max;
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index a1bd1be09548..f703bd3b7415 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -538,7 +538,7 @@ static struct slcan *slc_alloc(void)
dev->base_addr = i;
sl = netdev_priv(dev);
- dev->ml_priv = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN);
+ dev->can = (void *)sl + ALIGN(sizeof(*sl), NETDEV_ALIGN);
/* Initialize channel control data */
sl->magic = SLCAN_MAGIC;
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 39ca14b0585d..d8c8f9cc769f 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -153,7 +153,7 @@ static void vcan_setup(struct net_device *dev)
dev->addr_len = 0;
dev->tx_queue_len = 0;
dev->flags = IFF_NOARP;
- dev->ml_priv= netdev_priv(dev);
+ dev->can= netdev_priv(dev);
/* set flags according to driver capabilities */
if (echo)
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index fa47bab510bb..0fea82935bf0 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -147,7 +147,7 @@ static void vxcan_setup(struct net_device *dev)
dev->flags = (IFF_NOARP|IFF_ECHO);
dev->netdev_ops = _netdev_ops;
dev->needs_free_netdev = true;
- dev->ml_priv= netdev_priv(dev) + ALIGN(sizeof(struct
vxcan_priv), NETDEV_ALIGN);
+ dev->can= netdev_priv(dev) + ALIGN(sizeof(struct
vxcan_priv), NETDEV_ALIGN);
}
/* forward declaration for rtnl_create_link() */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5b949076ed23..825220da2e4f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2153,6 +2153,10 @@ struct net_device {
/* protected by rtnl_lock */
struct bpf_xdp_entity xdp_state[__MAX_XDP_MODE];
+
+#if IS_ENABLED(CONFIG_CAN)
+ struct can_ml_priv *can;
+#endif
};
#define to_net_dev(d) container_of(d, struct net_device, dev)
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 837bb8af0ec3..8651c8112a65 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -304,7 +304,7 @@ static struct can_dev_rcv_lists
*can_dev_rcv_lists_find(struct net *net,
struct net_device *dev)
{
if (dev) {
- struct can_ml_priv *ml_priv = dev->ml_priv;
+ struct can_ml_priv *ml_priv = dev->can;
return _priv->dev_rcv_lists;
} else {
return net->can.rx_alldev_list;
@@ -801,7 +801,7 @@ static int can_notifier(struct notifier_block *nb, unsigned
long msg,
switch (msg) {
case NETDEV_REGISTER:
- WARN(!dev->ml_priv,
+ WARN(!dev->can,
"No CAN mid layer private
Re: [PATCH net 1/6] net: dsa: ksz: fix FID management
On Wed, Jan 13, 2021 at 01:45:17PM +0100, Gilles DOFFE wrote: > The FID (Filter ID) is a 7 bits field used to link the VLAN table > to the static and dynamic mac address tables. > Until now the KSZ8795 driver could only add one VLAN as the FID was > always set to 1. What do you mean the ksz8769 driver could only add one VLAN? That is obviously a false statement. All VLANs use the same FID of 1 means that the switch is currently configured for shared address learning. Whereas each VLAN having a separate FID would mean that it is configured for individual address learning. > This commit allows setting a FID for each new active VLAN. > The FID list is stored in a static table dynamically allocated from > ks8795_fid structure. > Each newly activated VLAN is associated to the next available FID. > Only the VLAN 0 is not added to the list as it is a special VLAN. > As it has a special meaning, see IEEE 802.1q. > When a VLAN is no more used, the associated FID table entry is reset > to 0. Why is this patch targeting the "net" tree? What is the problem that it resolves?
Re: [PATCH net 1/6] net: dsa: ksz: fix FID management
- Le 13 Jan 21, à 13:45, Gilles Doffe gilles.do...@savoirfairelinux.com a
écrit :
> The FID (Filter ID) is a 7 bits field used to link the VLAN table
> to the static and dynamic mac address tables.
> Until now the KSZ8795 driver could only add one VLAN as the FID was
> always set to 1.
> This commit allows setting a FID for each new active VLAN.
> The FID list is stored in a static table dynamically allocated from
> ks8795_fid structure.
> Each newly activated VLAN is associated to the next available FID.
> Only the VLAN 0 is not added to the list as it is a special VLAN.
> As it has a special meaning, see IEEE 802.1q.
> When a VLAN is no more used, the associated FID table entry is reset
> to 0.
>
> Signed-off-by: Gilles DOFFE
> ---
> drivers/net/dsa/microchip/ksz8795.c | 59 +++--
> drivers/net/dsa/microchip/ksz8795_reg.h | 1 +
> drivers/net/dsa/microchip/ksz_common.h | 1 +
> 3 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index c973db101b72..6962ba4ee125 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -648,7 +648,7 @@ static enum dsa_tag_protocol
> ksz8795_get_tag_protocol(struct
> dsa_switch *ds,
> int port,
> enum dsa_tag_protocol mp)
> {
> - return DSA_TAG_PROTO_KSZ8795;
> + return DSA_TAG_PROTO_NONE;
This is an oversight, will be removed in V2.
> }
>
> static void ksz8795_get_strings(struct dsa_switch *ds, int port,
> @@ -796,6 +796,41 @@ static int ksz8795_port_vlan_filtering(struct dsa_switch
> *ds, int port,
> return 0;
> }
>
> +static void ksz8795_del_fid(u16 *ksz_fid_table, u16 vid)
> +{
> + u8 i = 0;
> +
> + if (!ksz_fid_table)
> + return;
> +
> + for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
> + if (ksz_fid_table[i] == vid) {
> + ksz_fid_table[i] = 0;
> + break;
> + }
> + }
> +}
> +
> +static int ksz8795_get_next_fid(u16 *ksz_fid_table, u16 vid, u8 *fid)
> +{
> + u8 i = 0;
> + int ret = -EOVERFLOW;
> +
> + if (!ksz_fid_table)
> + return ret;
> +
> + for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
> + if (!ksz_fid_table[i]) {
> + ksz_fid_table[i] = vid;
> + *fid = i;
> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
> const struct switchdev_obj_port_vlan *vlan)
> {
> @@ -803,17 +838,24 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds,
> int port,
> struct ksz_device *dev = ds->priv;
> u16 data, vid, new_pvid = 0;
> u8 fid, member, valid;
> + int ret;
>
> ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
>
> for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
> + if (vid == 0)
> + continue;
> +
> ksz8795_r_vlan_table(dev, vid, );
> ksz8795_from_vlan(data, , , );
>
> /* First time to setup the VLAN entry. */
> if (!valid) {
> - /* Need to find a way to map VID to FID. */
> - fid = 1;
> + ret = ksz8795_get_next_fid(dev->ksz_fid_table, vid,
> );
> + if (ret) {
> + dev_err(ds->dev, "Switch FID table is full,
> cannot add");
> + return;
> + }
> valid = 1;
> }
> member |= BIT(port);
> @@ -855,7 +897,7 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds,
> int
> port,
>
> /* Invalidate the entry if no more member. */
> if (!member) {
> - fid = 0;
> + ksz8795_del_fid(dev->ksz_fid_table, vid);
> valid = 0;
> }
>
> @@ -1087,6 +1129,9 @@ static int ksz8795_setup(struct dsa_switch *ds)
> for (i = 0; i < (dev->num_vlans / 4); i++)
> ksz8795_r_vlan_entries(dev, i);
>
> + /* Assign first FID to VLAN 1 and always return FID=0 for this vlan */
> + dev->ksz_fid_table[0] = 1;
> +
> /* Setup STP address for STP operation. */
> memset(, 0, sizeof(alu));
> ether_addr_copy(alu.mac, eth_stp_addr);
> @@ -1261,6 +1306,12 @@ static int ksz8795_switch_init(struct ksz_device *dev)
> /* set the real number of ports */
> dev->ds->num_ports = dev->port_cnt;
>
> + dev->ksz_fid_table = devm_kzalloc(dev->dev,
> + VLAN_TABLE_FID_SIZE * sizeof(u16),
> + GFP_KERNEL);
> + if
[PATCH net 1/6] net: dsa: ksz: fix FID management
The FID (Filter ID) is a 7 bits field used to link the VLAN table
to the static and dynamic mac address tables.
Until now the KSZ8795 driver could only add one VLAN as the FID was
always set to 1.
This commit allows setting a FID for each new active VLAN.
The FID list is stored in a static table dynamically allocated from
ks8795_fid structure.
Each newly activated VLAN is associated to the next available FID.
Only the VLAN 0 is not added to the list as it is a special VLAN.
As it has a special meaning, see IEEE 802.1q.
When a VLAN is no more used, the associated FID table entry is reset
to 0.
Signed-off-by: Gilles DOFFE
---
drivers/net/dsa/microchip/ksz8795.c | 59 +++--
drivers/net/dsa/microchip/ksz8795_reg.h | 1 +
drivers/net/dsa/microchip/ksz_common.h | 1 +
3 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz8795.c
b/drivers/net/dsa/microchip/ksz8795.c
index c973db101b72..6962ba4ee125 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -648,7 +648,7 @@ static enum dsa_tag_protocol
ksz8795_get_tag_protocol(struct dsa_switch *ds,
int port,
enum dsa_tag_protocol mp)
{
- return DSA_TAG_PROTO_KSZ8795;
+ return DSA_TAG_PROTO_NONE;
}
static void ksz8795_get_strings(struct dsa_switch *ds, int port,
@@ -796,6 +796,41 @@ static int ksz8795_port_vlan_filtering(struct dsa_switch
*ds, int port,
return 0;
}
+static void ksz8795_del_fid(u16 *ksz_fid_table, u16 vid)
+{
+ u8 i = 0;
+
+ if (!ksz_fid_table)
+ return;
+
+ for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
+ if (ksz_fid_table[i] == vid) {
+ ksz_fid_table[i] = 0;
+ break;
+ }
+ }
+}
+
+static int ksz8795_get_next_fid(u16 *ksz_fid_table, u16 vid, u8 *fid)
+{
+ u8 i = 0;
+ int ret = -EOVERFLOW;
+
+ if (!ksz_fid_table)
+ return ret;
+
+ for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
+ if (!ksz_fid_table[i]) {
+ ksz_fid_table[i] = vid;
+ *fid = i;
+ ret = 0;
+ break;
+ }
+ }
+
+ return ret;
+}
+
static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan)
{
@@ -803,17 +838,24 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds,
int port,
struct ksz_device *dev = ds->priv;
u16 data, vid, new_pvid = 0;
u8 fid, member, valid;
+ int ret;
ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
+ if (vid == 0)
+ continue;
+
ksz8795_r_vlan_table(dev, vid, );
ksz8795_from_vlan(data, , , );
/* First time to setup the VLAN entry. */
if (!valid) {
- /* Need to find a way to map VID to FID. */
- fid = 1;
+ ret = ksz8795_get_next_fid(dev->ksz_fid_table, vid,
);
+ if (ret) {
+ dev_err(ds->dev, "Switch FID table is full,
cannot add");
+ return;
+ }
valid = 1;
}
member |= BIT(port);
@@ -855,7 +897,7 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds, int
port,
/* Invalidate the entry if no more member. */
if (!member) {
- fid = 0;
+ ksz8795_del_fid(dev->ksz_fid_table, vid);
valid = 0;
}
@@ -1087,6 +1129,9 @@ static int ksz8795_setup(struct dsa_switch *ds)
for (i = 0; i < (dev->num_vlans / 4); i++)
ksz8795_r_vlan_entries(dev, i);
+ /* Assign first FID to VLAN 1 and always return FID=0 for this vlan */
+ dev->ksz_fid_table[0] = 1;
+
/* Setup STP address for STP operation. */
memset(, 0, sizeof(alu));
ether_addr_copy(alu.mac, eth_stp_addr);
@@ -1261,6 +1306,12 @@ static int ksz8795_switch_init(struct ksz_device *dev)
/* set the real number of ports */
dev->ds->num_ports = dev->port_cnt;
+ dev->ksz_fid_table = devm_kzalloc(dev->dev,
+ VLAN_TABLE_FID_SIZE * sizeof(u16),
+ GFP_KERNEL);
+ if (!dev->ksz_fid_table)
+ return -ENOMEM;
+
return 0;
}
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h
b/drivers/net/dsa/microchip/ksz8795_reg.h
index 40372047d40d..fe4c4f7fdd47 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
[PATCH net 1/2] net: ipa: introduce atomic channel STOPPING flag
Introduce a new atomic flag bit to communicate that a channel is
stopping. At the end of the NAPI poll loop, we normally re-enable
the IEOB interrupt, but now we won't do that if the channel is being
stopped. This is required for the next patch.
Fixes: 650d1603825d8 ("soc: qcom: ipa: the generic software interface")
Signed-off-by: Alex Elder
---
drivers/net/ipa/gsi.c | 11 ++-
drivers/net/ipa/gsi.h | 6 ++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 14d9a791924bf..7e7629902911e 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -739,6 +739,10 @@ static void gsi_channel_freeze(struct gsi_channel *channel)
{
gsi_channel_trans_quiesce(channel);
+ /* Don't let the NAPI poll loop re-enable interrupts when done */
+ set_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags);
+ smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */
+
napi_disable(>napi);
gsi_irq_ieob_disable(channel->gsi, channel->evt_ring_id);
@@ -749,6 +753,10 @@ static void gsi_channel_thaw(struct gsi_channel *channel)
{
gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id);
+ /* Allow the NAPI poll loop to re-enable interrupts again */
+ clear_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags);
+ smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */
+
napi_enable(>napi);
}
@@ -1536,7 +1544,8 @@ static int gsi_channel_poll(struct napi_struct *napi, int
budget)
if (count < budget) {
napi_complete(>napi);
- gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id);
+ if (!test_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags))
+ gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id);
}
return count;
diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
index 96c9aed397aad..8f0ae97c80c6e 100644
--- a/drivers/net/ipa/gsi.h
+++ b/drivers/net/ipa/gsi.h
@@ -104,9 +104,15 @@ enum gsi_channel_state {
GSI_CHANNEL_STATE_ERROR = 0xf,
};
+enum gsi_channel_flag {
+ GSI_CHANNEL_FLAG_STOPPING,
+ GSI_CHANNEL_FLAG_COUNT, /* Last; not a flag */
+};
+
/* We only care about channels between IPA and AP */
struct gsi_channel {
struct gsi *gsi;
+ DECLARE_BITMAP(flags, GSI_CHANNEL_FLAG_COUNT);
bool toward_ipa;
bool command; /* AP command TX channel or not */
--
2.20.1
Re: [PATCH net 1/4] net: stmmac: dwmac-sun8i: Fix probe error handling
On Sun, Jan 3, 2021 at 7:17 PM Samuel Holland wrote:
>
> stmmac_pltfr_remove does three things in one function, making it
> inapproprate for unwinding the steps in the probe function. Currently,
> a failure before the call to stmmac_dvr_probe would leak OF node
> references due to missing a call to stmmac_remove_config_dt. And an
> error in stmmac_dvr_probe would cause the driver to attempt to remove a
> netdevice that was never added. Fix these by reordering the init and
> splitting out the error handling steps.
>
> Fixes: 9f93ac8d4085 ("net-next: stmmac: Add dwmac-sun8i")
> Fixes: 40a1dcee2d18 ("net: ethernet: dwmac-sun8i: Use the correct function in
> exit path")
> Signed-off-by: Samuel Holland
Reviewed-by: Chen-Yu Tsai
Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets
The 12/28/2020 14:24, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:
> > Wireshark says that the MRP test packets cannot be decoded - and the
> > reason for that is that there's a two-byte hole filled with garbage
> > between the "transitions" and "timestamp" members.
> >
> > So Wireshark decodes the two garbage bytes and the top two bytes of
> > the timestamp written by the kernel as the timestamp value (which thus
> > fluctuates wildly), and interprets the lower two bytes of the
> > timestamp as a new (type, length) pair, which is of course broken.
> >
> > While my copy of the MRP standard is still under way [*], I cannot
> > imagine the standard specifying a two-byte hole here, and whoever
> > wrote the Wireshark decoding code seems to agree with that.
> >
> > The struct definitions live under include/uapi/, but they are not
> > really part of any kernel<->userspace API/ABI, so fixing the
> > definitions by adding the packed attribute should not cause any
> > compatibility issues.
> >
> > The remaining on-the-wire packet formats likely also don't contain
> > holes, but pahole and manual inspection says the current definitions
> > suffice. So adding the packed attribute to those is not strictly
> > needed, but might be done for good measure.
> >
> > [*] I will never understand how something hidden behind a +1000$
> > paywall can be called a standard.
> >
> > Signed-off-by: Rasmus Villemoes
> > ---
> > include/uapi/linux/mrp_bridge.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/mrp_bridge.h
> > b/include/uapi/linux/mrp_bridge.h
> > index 6aeb13ef0b1e..d1d0cf65916d 100644
> > --- a/include/uapi/linux/mrp_bridge.h
> > +++ b/include/uapi/linux/mrp_bridge.h
> > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> > __be16 state;
> > __be16 transitions;
> > __be32 timestamp;
> > -};
> > +} __attribute__((__packed__));
> >
> > struct br_mrp_ring_topo_hdr {
> > __be16 prio;
> > @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
> > __be16 state;
> > __be16 transitions;
> > __be32 timestamp;
> > -};
> > +} __attribute__((__packed__));
> >
> > struct br_mrp_in_topo_hdr {
> > __u8 sa[ETH_ALEN];
>
> Can we use this opportunity to move the definitions of these structures
> out of the uAPI to a normal kernel header?
Or maybe we can just remove them, especially if they are not used by the
kernel.
--
/Horatiu
[PATCH net 1/4] net: stmmac: dwmac-sun8i: Fix probe error handling
stmmac_pltfr_remove does three things in one function, making it
inapproprate for unwinding the steps in the probe function. Currently,
a failure before the call to stmmac_dvr_probe would leak OF node
references due to missing a call to stmmac_remove_config_dt. And an
error in stmmac_dvr_probe would cause the driver to attempt to remove a
netdevice that was never added. Fix these by reordering the init and
splitting out the error handling steps.
Fixes: 9f93ac8d4085 ("net-next: stmmac: Add dwmac-sun8i")
Fixes: 40a1dcee2d18 ("net: ethernet: dwmac-sun8i: Use the correct function in
exit path")
Signed-off-by: Samuel Holland
---
.../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 25 +++
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 58e0511badba..b20f261fce5b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -1134,10 +1134,6 @@ static int sun8i_dwmac_probe(struct platform_device
*pdev)
if (ret)
return ret;
- plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
- if (IS_ERR(plat_dat))
- return PTR_ERR(plat_dat);
-
gmac = devm_kzalloc(dev, sizeof(*gmac), GFP_KERNEL);
if (!gmac)
return -ENOMEM;
@@ -1201,11 +1197,15 @@ static int sun8i_dwmac_probe(struct platform_device
*pdev)
ret = of_get_phy_mode(dev->of_node, );
if (ret)
return -EINVAL;
- plat_dat->interface = interface;
+
+ plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
+ if (IS_ERR(plat_dat))
+ return PTR_ERR(plat_dat);
/* platform data specifying hardware features and callbacks.
* hardware features were copied from Allwinner drivers.
*/
+ plat_dat->interface = interface;
plat_dat->rx_coe = STMMAC_RX_COE_TYPE2;
plat_dat->tx_coe = 1;
plat_dat->has_sun8i = true;
@@ -1216,7 +1216,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
ret = sun8i_dwmac_init(pdev, plat_dat->bsp_priv);
if (ret)
- return ret;
+ goto dwmac_deconfig;
ret = stmmac_dvr_probe(>dev, plat_dat, _res);
if (ret)
@@ -1230,7 +1230,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
if (gmac->variant->soc_has_internal_phy) {
ret = get_ephy_nodes(priv);
if (ret)
- goto dwmac_exit;
+ goto dwmac_remove;
ret = sun8i_dwmac_register_mdio_mux(priv);
if (ret) {
dev_err(>dev, "Failed to register mux\n");
@@ -1239,15 +1239,20 @@ static int sun8i_dwmac_probe(struct platform_device
*pdev)
} else {
ret = sun8i_dwmac_reset(priv);
if (ret)
- goto dwmac_exit;
+ goto dwmac_remove;
}
return ret;
dwmac_mux:
sun8i_dwmac_unset_syscon(gmac);
+dwmac_remove:
+ stmmac_dvr_remove(>dev);
dwmac_exit:
- stmmac_pltfr_remove(pdev);
-return ret;
+ sun8i_dwmac_exit(pdev, gmac);
+dwmac_deconfig:
+ stmmac_remove_config_dt(pdev, plat_dat);
+
+ return ret;
}
static const struct of_device_id sun8i_dwmac_match[] = {
--
2.26.2
Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets
On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote:
> Wireshark says that the MRP test packets cannot be decoded - and the
> reason for that is that there's a two-byte hole filled with garbage
> between the "transitions" and "timestamp" members.
>
> So Wireshark decodes the two garbage bytes and the top two bytes of
> the timestamp written by the kernel as the timestamp value (which thus
> fluctuates wildly), and interprets the lower two bytes of the
> timestamp as a new (type, length) pair, which is of course broken.
>
> While my copy of the MRP standard is still under way [*], I cannot
> imagine the standard specifying a two-byte hole here, and whoever
> wrote the Wireshark decoding code seems to agree with that.
>
> The struct definitions live under include/uapi/, but they are not
> really part of any kernel<->userspace API/ABI, so fixing the
> definitions by adding the packed attribute should not cause any
> compatibility issues.
>
> The remaining on-the-wire packet formats likely also don't contain
> holes, but pahole and manual inspection says the current definitions
> suffice. So adding the packed attribute to those is not strictly
> needed, but might be done for good measure.
>
> [*] I will never understand how something hidden behind a +1000$
> paywall can be called a standard.
>
> Signed-off-by: Rasmus Villemoes
> ---
> include/uapi/linux/mrp_bridge.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
> index 6aeb13ef0b1e..d1d0cf65916d 100644
> --- a/include/uapi/linux/mrp_bridge.h
> +++ b/include/uapi/linux/mrp_bridge.h
> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> __be16 state;
> __be16 transitions;
> __be32 timestamp;
> -};
> +} __attribute__((__packed__));
>
> struct br_mrp_ring_topo_hdr {
> __be16 prio;
> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
> __be16 state;
> __be16 transitions;
> __be32 timestamp;
> -};
> +} __attribute__((__packed__));
>
> struct br_mrp_in_topo_hdr {
> __u8 sa[ETH_ALEN];
Can we use this opportunity to move the definitions of these structures
out of the uAPI to a normal kernel header?
[PATCH net 1/2] net: ipa: don't return a value from gsi_channel_command()
Callers of gsi_channel_command() no longer care whether the command
times out, and don't use what gsi_channel_command() returns. Redefine
that function to have void return type.
Reported-by: kernel test robot
Fixes: 6ffddf3b3d182 ("net: ipa: use state to determine channel command
success")
Signed-off-by: Alex Elder
---
drivers/net/ipa/gsi.c | 21 +++--
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 579cc3e516775..e51a770578990 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -454,7 +454,7 @@ static enum gsi_channel_state gsi_channel_state(struct
gsi_channel *channel)
}
/* Issue a channel command and wait for it to complete */
-static int
+static void
gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode)
{
struct completion *completion = >completion;
@@ -489,12 +489,10 @@ gsi_channel_command(struct gsi_channel *channel, enum
gsi_ch_cmd_opcode opcode)
iowrite32(0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
if (success)
- return 0;
+ return;
dev_err(dev, "GSI command %u for channel %u timed out, state %u\n",
opcode, channel_id, gsi_channel_state(channel));
-
- return -ETIMEDOUT;
}
/* Allocate GSI channel in NOT_ALLOCATED state */
@@ -503,7 +501,6 @@ static int gsi_channel_alloc_command(struct gsi *gsi, u32
channel_id)
struct gsi_channel *channel = >channel[channel_id];
struct device *dev = gsi->dev;
enum gsi_channel_state state;
- int ret;
/* Get initial channel state */
state = gsi_channel_state(channel);
@@ -513,7 +510,7 @@ static int gsi_channel_alloc_command(struct gsi *gsi, u32
channel_id)
return -EINVAL;
}
- ret = gsi_channel_command(channel, GSI_CH_ALLOCATE);
+ gsi_channel_command(channel, GSI_CH_ALLOCATE);
/* If successful the channel state will have changed */
state = gsi_channel_state(channel);
@@ -531,7 +528,6 @@ static int gsi_channel_start_command(struct gsi_channel
*channel)
{
struct device *dev = channel->gsi->dev;
enum gsi_channel_state state;
- int ret;
state = gsi_channel_state(channel);
if (state != GSI_CHANNEL_STATE_ALLOCATED &&
@@ -541,7 +537,7 @@ static int gsi_channel_start_command(struct gsi_channel
*channel)
return -EINVAL;
}
- ret = gsi_channel_command(channel, GSI_CH_START);
+ gsi_channel_command(channel, GSI_CH_START);
/* If successful the channel state will have changed */
state = gsi_channel_state(channel);
@@ -559,7 +555,6 @@ static int gsi_channel_stop_command(struct gsi_channel
*channel)
{
struct device *dev = channel->gsi->dev;
enum gsi_channel_state state;
- int ret;
state = gsi_channel_state(channel);
@@ -576,7 +571,7 @@ static int gsi_channel_stop_command(struct gsi_channel
*channel)
return -EINVAL;
}
- ret = gsi_channel_command(channel, GSI_CH_STOP);
+ gsi_channel_command(channel, GSI_CH_STOP);
/* If successful the channel state will have changed */
state = gsi_channel_state(channel);
@@ -598,7 +593,6 @@ static void gsi_channel_reset_command(struct gsi_channel
*channel)
{
struct device *dev = channel->gsi->dev;
enum gsi_channel_state state;
- int ret;
msleep(1); /* A short delay is required before a RESET command */
@@ -612,7 +606,7 @@ static void gsi_channel_reset_command(struct gsi_channel
*channel)
return;
}
- ret = gsi_channel_command(channel, GSI_CH_RESET);
+ gsi_channel_command(channel, GSI_CH_RESET);
/* If successful the channel state will have changed */
state = gsi_channel_state(channel);
@@ -627,7 +621,6 @@ static void gsi_channel_de_alloc_command(struct gsi *gsi,
u32 channel_id)
struct gsi_channel *channel = >channel[channel_id];
struct device *dev = gsi->dev;
enum gsi_channel_state state;
- int ret;
state = gsi_channel_state(channel);
if (state != GSI_CHANNEL_STATE_ALLOCATED) {
@@ -636,7 +629,7 @@ static void gsi_channel_de_alloc_command(struct gsi *gsi,
u32 channel_id)
return;
}
- ret = gsi_channel_command(channel, GSI_CH_DE_ALLOC);
+ gsi_channel_command(channel, GSI_CH_DE_ALLOC);
/* If successful the channel state will have changed */
state = gsi_channel_state(channel);
--
2.27.0
Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets
Hi Andrew,
The 12/23/2020 19:41, Andrew Lunn wrote:
>
> > > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> > > __be16 state;
> > > __be16 transitions;
> > > __be32 timestamp;
> > > -};
> > > +} __attribute__((__packed__));
> >
> > Yes, I agree that this should be packed but it also needs to be 32 bit
> > alligned, so extra 2 bytes are needed.
>
> The full structure is:
>
> struct br_mrp_ring_test_hdr {
> __be16 prio;
> __u8 sa[ETH_ALEN];
> __be16 port_role;
> __be16 state;
> __be16 transitions;
> __be32 timestamp;
> };
>
> If i'm looking at this correctly, the byte offsets are:
>
> 0-1 prio
> 2-7 sa
> 8-9 port_role
> 10-11 state
> 12-13 transition
>
> With packed you get
>
> 14-17 timestamp
>
> which is not 32 bit aligned.
>
> Do you mean the whole structure must be 32 bit aligned? We need to add
> two reserved bytes to the end of the structure?
Sorry, I looked too fast over this. First some info, in front of the
'br_mrp_ring_test_hdr', there is 'br_mrp_tlv_hdr' which is 2
bytes. So the frame will look something like this:
... |-||--||
... | MRP Ver | br_mrp_tlv_hdr | br_mrp_ring_test_hdr | Common TLV |
... |-||--||
The standard says that for br_mrp_tlv_hdr + br_mrp_ring_test, 32 bit
alignment shall be ensured. So my understanding was that it needs to be
at word boundary(4 bytes).
So based on this, if we use packed then we get the following offsets
0 type
1 length
2-3 prio
4-9 sa
10-11 port_role
12-13 state
14-15 transition
16-19 timestamp.
Which is 20 bytes, that fits correctly. So my understanding is we need to
use packed, to remove the hole between transition and timestamp as
Rasmus suggested and should NOT use these 2 extra bytes that I
mentioned because it would not be aligned anymore.
Here you can find few more details about MRP[1]
>
> Andrew
[1] https://www.youtube.com/watch?v=R3vQYfwiJ2M
--
/Horatiu
Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets
> > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> > __be16 state;
> > __be16 transitions;
> > __be32 timestamp;
> > -};
> > +} __attribute__((__packed__));
>
> Yes, I agree that this should be packed but it also needs to be 32 bit
> alligned, so extra 2 bytes are needed.
The full structure is:
struct br_mrp_ring_test_hdr {
__be16 prio;
__u8 sa[ETH_ALEN];
__be16 port_role;
__be16 state;
__be16 transitions;
__be32 timestamp;
};
If i'm looking at this correctly, the byte offsets are:
0-1 prio
2-7 sa
8-9 port_role
10-11 state
12-13 transition
With packed you get
14-17 timestamp
which is not 32 bit aligned.
Do you mean the whole structure must be 32 bit aligned? We need to add
two reserved bytes to the end of the structure?
Andrew
Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets
The 12/23/2020 15:45, Rasmus Villemoes wrote:
Hi Rasmus,
>
> Wireshark says that the MRP test packets cannot be decoded - and the
> reason for that is that there's a two-byte hole filled with garbage
> between the "transitions" and "timestamp" members.
>
> So Wireshark decodes the two garbage bytes and the top two bytes of
> the timestamp written by the kernel as the timestamp value (which thus
> fluctuates wildly), and interprets the lower two bytes of the
> timestamp as a new (type, length) pair, which is of course broken.
>
> While my copy of the MRP standard is still under way [*], I cannot
> imagine the standard specifying a two-byte hole here, and whoever
> wrote the Wireshark decoding code seems to agree with that.
>
> The struct definitions live under include/uapi/, but they are not
> really part of any kernel<->userspace API/ABI, so fixing the
> definitions by adding the packed attribute should not cause any
> compatibility issues.
>
> The remaining on-the-wire packet formats likely also don't contain
> holes, but pahole and manual inspection says the current definitions
> suffice. So adding the packed attribute to those is not strictly
> needed, but might be done for good measure.
>
> [*] I will never understand how something hidden behind a +1000$
> paywall can be called a standard.
>
> Signed-off-by: Rasmus Villemoes
> ---
> include/uapi/linux/mrp_bridge.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
> index 6aeb13ef0b1e..d1d0cf65916d 100644
> --- a/include/uapi/linux/mrp_bridge.h
> +++ b/include/uapi/linux/mrp_bridge.h
> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> __be16 state;
> __be16 transitions;
> __be32 timestamp;
> -};
> +} __attribute__((__packed__));
Yes, I agree that this should be packed but it also needs to be 32 bit
alligned, so extra 2 bytes are needed.
The same will apply also for 'br_mrp_ring_topo_hdr'
>
> struct br_mrp_ring_topo_hdr {
> __be16 prio;
> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
> __be16 state;
> __be16 transitions;
> __be32 timestamp;
> -};
> +} __attribute__((__packed__));
>
> struct br_mrp_in_topo_hdr {
> __u8 sa[ETH_ALEN];
> --
> 2.23.0
>
--
/Horatiu
[PATCH net 1/2] net: mrp: fix definitions of MRP test packets
Wireshark says that the MRP test packets cannot be decoded - and the
reason for that is that there's a two-byte hole filled with garbage
between the "transitions" and "timestamp" members.
So Wireshark decodes the two garbage bytes and the top two bytes of
the timestamp written by the kernel as the timestamp value (which thus
fluctuates wildly), and interprets the lower two bytes of the
timestamp as a new (type, length) pair, which is of course broken.
While my copy of the MRP standard is still under way [*], I cannot
imagine the standard specifying a two-byte hole here, and whoever
wrote the Wireshark decoding code seems to agree with that.
The struct definitions live under include/uapi/, but they are not
really part of any kernel<->userspace API/ABI, so fixing the
definitions by adding the packed attribute should not cause any
compatibility issues.
The remaining on-the-wire packet formats likely also don't contain
holes, but pahole and manual inspection says the current definitions
suffice. So adding the packed attribute to those is not strictly
needed, but might be done for good measure.
[*] I will never understand how something hidden behind a +1000$
paywall can be called a standard.
Signed-off-by: Rasmus Villemoes
---
include/uapi/linux/mrp_bridge.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
index 6aeb13ef0b1e..d1d0cf65916d 100644
--- a/include/uapi/linux/mrp_bridge.h
+++ b/include/uapi/linux/mrp_bridge.h
@@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
__be16 state;
__be16 transitions;
__be32 timestamp;
-};
+} __attribute__((__packed__));
struct br_mrp_ring_topo_hdr {
__be16 prio;
@@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr {
__be16 state;
__be16 transitions;
__be32 timestamp;
-};
+} __attribute__((__packed__));
struct br_mrp_in_topo_hdr {
__u8 sa[ETH_ALEN];
--
2.23.0
[PATCH net 1/3] net: ipa: clear pending interrupts before enabling
We enable the completion interrupt for channel or event ring
commands only when we issue them. The interrupt is disabled after
the interrupt has fired, or after we have timed out waiting for it.
If we time out, the command could complete after the interrupt has
been disabled, causing a state change in the channel or event ring.
The interrupt associated with that state change would be delivered
the next time the completion interrupt is enabled.
To avoid previous command completions interfering with new commands,
clear all pending completion interrupts before re-enabling them for
a new command.
Fixes: b4175f8731f78 ("net: ipa: only enable GSI event control IRQs when
needed")
Signed-off-by: Alex Elder
---
drivers/net/ipa/gsi.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index c4795249719d4..4aee60d62ab09 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -340,7 +340,13 @@ static int evt_ring_command(struct gsi *gsi, u32
evt_ring_id,
* is issued here. Only permit *this* event ring to trigger
* an interrupt, and only enable the event control IRQ type
* when we expect it to occur.
+*
+* There's a small chance that a previous command completed
+* after the interrupt was disabled, so make sure we have no
+* pending interrupts before we enable them.
*/
+ iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_CLR_OFFSET);
+
val = BIT(evt_ring_id);
iowrite32(val, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET);
gsi_irq_type_enable(gsi, GSI_EV_CTRL);
@@ -453,7 +459,13 @@ gsi_channel_command(struct gsi_channel *channel, enum
gsi_ch_cmd_opcode opcode)
* issued here. So we only permit *this* channel to trigger
* an interrupt and only enable the channel control IRQ type
* when we expect it to occur.
+*
+* There's a small chance that a previous command completed
+* after the interrupt was disabled, so make sure we have no
+* pending interrupts before we enable them.
*/
+ iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_CLR_OFFSET);
+
val = BIT(channel_id);
iowrite32(val, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
gsi_irq_type_enable(gsi, GSI_CH_CTRL);
--
2.20.1
RE: [EXT] Re: [PATCH net 1/2] net: mvpp2: Fix GoP port 3 Networking Complex Control configurations
> -Original Message- > From: Jakub Kicinski > Sent: Thursday, December 17, 2020 2:42 AM > To: Stefan Chulski > Cc: net...@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan Markman > ; linux-kernel@vger.kernel.org; > li...@armlinux.org.uk; m...@semihalf.com; and...@lunn.ch; > rmk+ker...@armlinux.org.uk > Subject: [EXT] Re: [PATCH net 1/2] net: mvpp2: Fix GoP port 3 Networking > Complex Control configurations > > External Email > > -- > On Tue, 15 Dec 2020 15:32:12 +0200 stef...@marvell.com wrote: > > From: Stefan Chulski > > > > During GoP port 2 Networking Complex Control mode of operation > > configurations, also GoP port 3 mode of operation was wrongly set mode. > > Patch removes these configurations. > > GENCONF_CTRL0_PORTX naming also fixed. > > Can we get a Fixes tag? Reposting with Fixes tag. Stefan.
Re: [PATCH net 1/2] net: mvpp2: Fix GoP port 3 Networking Complex Control configurations
On Tue, 15 Dec 2020 15:32:12 +0200 stef...@marvell.com wrote: > From: Stefan Chulski > > During GoP port 2 Networking Complex Control mode of operation configurations, > also GoP port 3 mode of operation was wrongly set mode. > Patch removes these configurations. > GENCONF_CTRL0_PORTX naming also fixed. Can we get a Fixes tag?
[PATCH net 1/2] net: mvpp2: Fix GoP port 3 Networking Complex Control configurations
From: Stefan Chulski
During GoP port 2 Networking Complex Control mode of operation configurations,
also GoP port 3 mode of operation was wrongly set mode.
Patch removes these configurations.
GENCONF_CTRL0_PORTX naming also fixed.
Signed-off-by: Stefan Chulski
---
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 6 +++---
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 8
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index 6bd7e40..39c4e5c 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -651,9 +651,9 @@
#define GENCONF_PORT_CTRL1_EN(p) BIT(p)
#define GENCONF_PORT_CTRL1_RESET(p)(BIT(p) << 28)
#define GENCONF_CTRL0 0x1120
-#define GENCONF_CTRL0_PORT0_RGMII BIT(0)
-#define GENCONF_CTRL0_PORT1_RGMII_MII BIT(1)
-#define GENCONF_CTRL0_PORT1_RGMII BIT(2)
+#define GENCONF_CTRL0_PORT2_RGMII BIT(0)
+#define GENCONF_CTRL0_PORT3_RGMII_MII BIT(1)
+#define GENCONF_CTRL0_PORT3_RGMII BIT(2)
/* Various constants */
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index d64dc12..d2b0506 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -1231,9 +1231,9 @@ static void mvpp22_gop_init_rgmii(struct mvpp2_port *port)
regmap_read(priv->sysctrl_base, GENCONF_CTRL0, );
if (port->gop_id == 2)
- val |= GENCONF_CTRL0_PORT0_RGMII | GENCONF_CTRL0_PORT1_RGMII;
+ val |= GENCONF_CTRL0_PORT2_RGMII;
else if (port->gop_id == 3)
- val |= GENCONF_CTRL0_PORT1_RGMII_MII;
+ val |= GENCONF_CTRL0_PORT3_RGMII_MII;
regmap_write(priv->sysctrl_base, GENCONF_CTRL0, val);
}
@@ -1250,9 +1250,9 @@ static void mvpp22_gop_init_sgmii(struct mvpp2_port *port)
if (port->gop_id > 1) {
regmap_read(priv->sysctrl_base, GENCONF_CTRL0, );
if (port->gop_id == 2)
- val &= ~GENCONF_CTRL0_PORT0_RGMII;
+ val &= ~GENCONF_CTRL0_PORT2_RGMII;
else if (port->gop_id == 3)
- val &= ~GENCONF_CTRL0_PORT1_RGMII_MII;
+ val &= ~GENCONF_CTRL0_PORT3_RGMII_MII;
regmap_write(priv->sysctrl_base, GENCONF_CTRL0, val);
}
}
--
1.9.1
RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
> -Original Message- > From: Patrick Havelange > Sent: 10 December 2020 12:06 > To: Madalin Bucur ; David S. Miller > ; Jakub Kicinski ; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource > region reservation > > On 2020-12-10 10:05, Madalin Bucur wrote: > >> -Original Message- > >> From: Patrick Havelange > > [snipped] > > >> > >> But then that change would not be compatible with the existing device > >> trees in already existing hardware. I'm not sure how to handle that > case > >> properly. > > > > One needs to be backwards compatible with the old device trees, so we do > not > > really have a simple answer, I know. > > > >>> If we want to hack it, > >>> instead of splitting ioremaps, we can reserve 4 kB in the FMan driver, > >>> and keep the ioremap as it is now, with the benefit of less code churn. > >> > >> but then the ioremap and the memory reservation do not match. Why > bother > >> at all then with the mem reservation, just ioremap only and be done > with > >> it. What I'm saying is, I don't see the point of having a "fake" > >> reservation call if it does not correspond that what is being used. > > > > The reservation is not fake, it just covering the first portion of the > ioremap. > > Another hypothetical FMan driver would presumably reserve and ioremap > starting > > from the same point, thus the desired error would be met. > > > > Regarding removing reservation altogether, yes, we can do that, in the > child > > device drivers. That will fix that use after free issue you've found and > align > > with the custom, hierarchical structure of the FMan devices/drivers. But > would > > leave them without the double use guard we have when using the > reservation. > > > >>> In the end, what the reservation is trying to achieve is to make sure > >> there > >>> is a single driver controlling a certain peripeheral, and this basic > >>> requirement would be addressed by that change plus devm_of_iomap() for > >> child > >>> devices (ports, MACs). > >> > >> Again, correct me if I'm wrong, but with the fake mem reservation, it > >> would *not* make sure that a single driver is controlling a certain > >> peripheral. > > > > Actually, it would. If the current FMan driver reserves the first part > of the FMan > > memory, then another FMan driver (I do not expect a random driver trying > to map the > > FMan register area) > > Ha!, now I understand your point. I still think it is not a clean > solution, as the reservation do not match the ioremap usage. > > > would likely try to use that same part (with the same or > > a different size, it does not matter, there will be an error anyway) and > the > > reservation attempt will fail. If we fix the child device drivers, then > they > > will have normal mappings and reservations. > > > >> My point is, either have a *correct* mem reservation, or don't have one > >> at all. There is no point in trying to cheat the system. > > > > Now we do not have correct reservations for the child devices because > the > > parent takes it all for himself. Reduce the parent reservation and make > room > > for correct reservations for the child. The two-sections change you've > made may > > try to be correct but it's overkill for the purpose of detecting double > use. > > But it is not overkill if we want to detect potential subdrivers mapping > sections that would not start at the main fman region (but still part of > the main fman region). > > > And I also find the patch to obfuscate the already not so readable code > so I'd > > opt for a simpler fix. > > As said already, I'm not in favor of having a reservation that do not > match the real usage. > > And in my opinion, having a mismatch with the mem reservation and the > mem usage is also the kind of obfuscation that we want to avoid. > > Yes now the code is slightly more complex, but it is also slightly more > correct. > > I'm not seeing currently another way on how to make it simpler *and* > correct at the same time. Ok, we've taken note on your report and will put together a fix. Regards, Madalin
Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
On 2020-12-10 10:05, Madalin Bucur wrote: -Original Message- From: Patrick Havelange [snipped] But then that change would not be compatible with the existing device trees in already existing hardware. I'm not sure how to handle that case properly. One needs to be backwards compatible with the old device trees, so we do not really have a simple answer, I know. If we want to hack it, instead of splitting ioremaps, we can reserve 4 kB in the FMan driver, and keep the ioremap as it is now, with the benefit of less code churn. but then the ioremap and the memory reservation do not match. Why bother at all then with the mem reservation, just ioremap only and be done with it. What I'm saying is, I don't see the point of having a "fake" reservation call if it does not correspond that what is being used. The reservation is not fake, it just covering the first portion of the ioremap. Another hypothetical FMan driver would presumably reserve and ioremap starting from the same point, thus the desired error would be met. Regarding removing reservation altogether, yes, we can do that, in the child device drivers. That will fix that use after free issue you've found and align with the custom, hierarchical structure of the FMan devices/drivers. But would leave them without the double use guard we have when using the reservation. In the end, what the reservation is trying to achieve is to make sure there is a single driver controlling a certain peripeheral, and this basic requirement would be addressed by that change plus devm_of_iomap() for child devices (ports, MACs). Again, correct me if I'm wrong, but with the fake mem reservation, it would *not* make sure that a single driver is controlling a certain peripheral. Actually, it would. If the current FMan driver reserves the first part of the FMan memory, then another FMan driver (I do not expect a random driver trying to map the FMan register area) Ha!, now I understand your point. I still think it is not a clean solution, as the reservation do not match the ioremap usage. would likely try to use that same part (with the same or a different size, it does not matter, there will be an error anyway) and the reservation attempt will fail. If we fix the child device drivers, then they will have normal mappings and reservations. My point is, either have a *correct* mem reservation, or don't have one at all. There is no point in trying to cheat the system. Now we do not have correct reservations for the child devices because the parent takes it all for himself. Reduce the parent reservation and make room for correct reservations for the child. The two-sections change you've made may try to be correct but it's overkill for the purpose of detecting double use. But it is not overkill if we want to detect potential subdrivers mapping sections that would not start at the main fman region (but still part of the main fman region). And I also find the patch to obfuscate the already not so readable code so I'd opt for a simpler fix. As said already, I'm not in favor of having a reservation that do not match the real usage. And in my opinion, having a mismatch with the mem reservation and the mem usage is also the kind of obfuscation that we want to avoid. Yes now the code is slightly more complex, but it is also slightly more correct. I'm not seeing currently another way on how to make it simpler *and* correct at the same time. Patrick H. Madalin
RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
> -Original Message- > From: Patrick Havelange > Sent: 10 December 2020 10:49 > To: Madalin Bucur ; David S. Miller > ; Jakub Kicinski ; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource > region reservation > > On 2020-12-09 19:55, Madalin Bucur wrote: > >> -Original Message- > >> From: Patrick Havelange > >> Sent: 09 December 2020 16:17 > >> To: Madalin Bucur ; David S. Miller > >> ; Jakub Kicinski ; > >> net...@vger.kernel.org; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main > resource > >> region reservation > >> > >>>>> area. I'm assuming this is the problem you are trying to address > here, > >>>>> besides the stack corruption issue. > >>>> > >>>> Yes exactly. > >>>> I did not add this behaviour (having a main region and subdrivers > using > >>>> subregions), I'm just trying to correct what is already there. > >>>> For example: this is some content of /proc/iomem for one board I'm > >>>> working with, with the current existing code: > >>>> ffe40-ffe4fdfff : fman > >>>> ffe4e-ffe4e0fff : mac > >>>> ffe4e2000-ffe4e2fff : mac > >>>> ffe4e4000-ffe4e4fff : mac > >>>> ffe4e6000-ffe4e6fff : mac > >>>> ffe4e8000-ffe4e8fff : mac > >>>> > >>>> and now with my patches: > >>>> ffe40-ffe4fdfff : /soc@ffe00/fman@40 > >>>> ffe40-ffe480fff : fman > >>>> ffe488000-ffe488fff : fman-port > >>>> ffe489000-ffe489fff : fman-port > >>>> ffe48a000-ffe48afff : fman-port > >>>> ffe48b000-ffe48bfff : fman-port > >>>> ffe48c000-ffe48cfff : fman-port > >>>> ffe4a8000-ffe4a8fff : fman-port > >>>> ffe4a9000-ffe4a9fff : fman-port > >>>> ffe4aa000-ffe4aafff : fman-port > >>>> ffe4ab000-ffe4abfff : fman-port > >>>> ffe4ac000-ffe4acfff : fman-port > >>>> ffe4c-ffe4d : fman > >>>> ffe4e-ffe4e0fff : mac > >>>> ffe4e2000-ffe4e2fff : mac > >>>> ffe4e4000-ffe4e4fff : mac > >>>> ffe4e6000-ffe4e6fff : mac > >>>> ffe4e8000-ffe4e8fff : mac > >>>> > >>>>> While for the latter I think we can > >>>>> put together a quick fix, for the former I'd like to take a bit of > >> time > >>>>> to select the best fix, if one is really needed. So, please, let's > >> split > >>>>> the two problems and first address the incorrect stack memory use. > >>>> > >>>> I have no idea how you can fix it without a (more correct this time) > >>>> dummy region passed as parameter (and you don't want to use the first > >>>> patch). But then it will be useless to do the call anyway, as it > won't > >>>> do any proper verification at all, so it could also be removed > entirely, > >>>> which begs the question, why do it at all in the first place (the > >>>> devm_request_mem_region). > >>>> > >>>> I'm not an expert in that part of the code so feel free to correct me > >> if > >>>> I missed something. > >>>> > >>>> BR, > >>>> > >>>> Patrick H. > >>> > >>> Hi, Patrick, > >>> > >>> the DPAA entities are described in the device tree. Adding some > >> hardcoding in > >>> the driver is not really the solution for this problem. And I'm not > sure > >> we have > >> > >> I'm not seeing any problem here, the offsets used by the fman driver > >> were already there, I just reorganized them in 2 blocks. > >> > >>> a clear problem statement to start with. Can you help me on that part? > >> > >> - The current call to __devm_request_region in fman_port.c is not > correct. > >> > >> One way to fix this is to use devm_request_mem_region, however this > >> requires that the main fman would not be reserving the whole region. > >> This leads to the second problem: > >> - Make sure the main fman driver is not reserving the whole region. > >> > >>
Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
On 2020-12-09 19:55, Madalin Bucur wrote: -Original Message- From: Patrick Havelange Sent: 09 December 2020 16:17 To: Madalin Bucur ; David S. Miller ; Jakub Kicinski ; net...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation area. I'm assuming this is the problem you are trying to address here, besides the stack corruption issue. Yes exactly. I did not add this behaviour (having a main region and subdrivers using subregions), I'm just trying to correct what is already there. For example: this is some content of /proc/iomem for one board I'm working with, with the current existing code: ffe40-ffe4fdfff : fman ffe4e-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac and now with my patches: ffe40-ffe4fdfff : /soc@ffe00/fman@40 ffe40-ffe480fff : fman ffe488000-ffe488fff : fman-port ffe489000-ffe489fff : fman-port ffe48a000-ffe48afff : fman-port ffe48b000-ffe48bfff : fman-port ffe48c000-ffe48cfff : fman-port ffe4a8000-ffe4a8fff : fman-port ffe4a9000-ffe4a9fff : fman-port ffe4aa000-ffe4aafff : fman-port ffe4ab000-ffe4abfff : fman-port ffe4ac000-ffe4acfff : fman-port ffe4c-ffe4d : fman ffe4e-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac While for the latter I think we can put together a quick fix, for the former I'd like to take a bit of time to select the best fix, if one is really needed. So, please, let's split the two problems and first address the incorrect stack memory use. I have no idea how you can fix it without a (more correct this time) dummy region passed as parameter (and you don't want to use the first patch). But then it will be useless to do the call anyway, as it won't do any proper verification at all, so it could also be removed entirely, which begs the question, why do it at all in the first place (the devm_request_mem_region). I'm not an expert in that part of the code so feel free to correct me if I missed something. BR, Patrick H. Hi, Patrick, the DPAA entities are described in the device tree. Adding some hardcoding in the driver is not really the solution for this problem. And I'm not sure we have I'm not seeing any problem here, the offsets used by the fman driver were already there, I just reorganized them in 2 blocks. a clear problem statement to start with. Can you help me on that part? - The current call to __devm_request_region in fman_port.c is not correct. One way to fix this is to use devm_request_mem_region, however this requires that the main fman would not be reserving the whole region. This leads to the second problem: - Make sure the main fman driver is not reserving the whole region. Is that clearer like this ? Patrick H. Hi, The overlapping IO areas result from the device tree description, that in turn mimics the HW description in the manual. If we really want to remove the nesting, we should change the device trees, not the drivers. But then that change would not be compatible with the existing device trees in already existing hardware. I'm not sure how to handle that case properly. If we want to hack it, instead of splitting ioremaps, we can reserve 4 kB in the FMan driver, and keep the ioremap as it is now, with the benefit of less code churn. but then the ioremap and the memory reservation do not match. Why bother at all then with the mem reservation, just ioremap only and be done with it. What I'm saying is, I don't see the point of having a "fake" reservation call if it does not correspond that what is being used. In the end, what the reservation is trying to achieve is to make sure there is a single driver controlling a certain peripeheral, and this basic requirement would be addressed by that change plus devm_of_iomap() for child devices (ports, MACs). Again, correct me if I'm wrong, but with the fake mem reservation, it would *not* make sure that a single driver is controlling a certain peripheral. My point is, either have a *correct* mem reservation, or don't have one at all. There is no point in trying to cheat the system. Patrick H. Madalin
RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
> -Original Message- > From: Patrick Havelange > Sent: 09 December 2020 16:17 > To: Madalin Bucur ; David S. Miller > ; Jakub Kicinski ; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource > region reservation > > >>> area. I'm assuming this is the problem you are trying to address here, > >>> besides the stack corruption issue. > >> > >> Yes exactly. > >> I did not add this behaviour (having a main region and subdrivers using > >> subregions), I'm just trying to correct what is already there. > >> For example: this is some content of /proc/iomem for one board I'm > >> working with, with the current existing code: > >> ffe40-ffe4fdfff : fman > >> ffe4e-ffe4e0fff : mac > >> ffe4e2000-ffe4e2fff : mac > >> ffe4e4000-ffe4e4fff : mac > >> ffe4e6000-ffe4e6fff : mac > >> ffe4e8000-ffe4e8fff : mac > >> > >> and now with my patches: > >> ffe40-ffe4fdfff : /soc@ffe00/fman@40 > >> ffe40-ffe480fff : fman > >> ffe488000-ffe488fff : fman-port > >> ffe489000-ffe489fff : fman-port > >> ffe48a000-ffe48afff : fman-port > >> ffe48b000-ffe48bfff : fman-port > >> ffe48c000-ffe48cfff : fman-port > >> ffe4a8000-ffe4a8fff : fman-port > >> ffe4a9000-ffe4a9fff : fman-port > >> ffe4aa000-ffe4aafff : fman-port > >> ffe4ab000-ffe4abfff : fman-port > >> ffe4ac000-ffe4acfff : fman-port > >> ffe4c-ffe4d : fman > >> ffe4e-ffe4e0fff : mac > >> ffe4e2000-ffe4e2fff : mac > >> ffe4e4000-ffe4e4fff : mac > >> ffe4e6000-ffe4e6fff : mac > >> ffe4e8000-ffe4e8fff : mac > >> > >>> While for the latter I think we can > >>> put together a quick fix, for the former I'd like to take a bit of > time > >>> to select the best fix, if one is really needed. So, please, let's > split > >>> the two problems and first address the incorrect stack memory use. > >> > >> I have no idea how you can fix it without a (more correct this time) > >> dummy region passed as parameter (and you don't want to use the first > >> patch). But then it will be useless to do the call anyway, as it won't > >> do any proper verification at all, so it could also be removed entirely, > >> which begs the question, why do it at all in the first place (the > >> devm_request_mem_region). > >> > >> I'm not an expert in that part of the code so feel free to correct me > if > >> I missed something. > >> > >> BR, > >> > >> Patrick H. > > > > Hi, Patrick, > > > > the DPAA entities are described in the device tree. Adding some > hardcoding in > > the driver is not really the solution for this problem. And I'm not sure > we have > > I'm not seeing any problem here, the offsets used by the fman driver > were already there, I just reorganized them in 2 blocks. > > > a clear problem statement to start with. Can you help me on that part? > > - The current call to __devm_request_region in fman_port.c is not correct. > > One way to fix this is to use devm_request_mem_region, however this > requires that the main fman would not be reserving the whole region. > This leads to the second problem: > - Make sure the main fman driver is not reserving the whole region. > > Is that clearer like this ? > > Patrick H. The overlapping IO areas result from the device tree description, that in turn mimics the HW description in the manual. If we really want to remove the nesting, we should change the device trees, not the drivers. If we want to hack it, instead of splitting ioremaps, we can reserve 4 kB in the FMan driver, and keep the ioremap as it is now, with the benefit of less code churn. In the end, what the reservation is trying to achieve is to make sure there is a single driver controlling a certain peripeheral, and this basic requirement would be addressed by that change plus devm_of_iomap() for child devices (ports, MACs). Madalin
Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
area. I'm assuming this is the problem you are trying to address here, besides the stack corruption issue. Yes exactly. I did not add this behaviour (having a main region and subdrivers using subregions), I'm just trying to correct what is already there. For example: this is some content of /proc/iomem for one board I'm working with, with the current existing code: ffe40-ffe4fdfff : fman ffe4e-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac and now with my patches: ffe40-ffe4fdfff : /soc@ffe00/fman@40 ffe40-ffe480fff : fman ffe488000-ffe488fff : fman-port ffe489000-ffe489fff : fman-port ffe48a000-ffe48afff : fman-port ffe48b000-ffe48bfff : fman-port ffe48c000-ffe48cfff : fman-port ffe4a8000-ffe4a8fff : fman-port ffe4a9000-ffe4a9fff : fman-port ffe4aa000-ffe4aafff : fman-port ffe4ab000-ffe4abfff : fman-port ffe4ac000-ffe4acfff : fman-port ffe4c-ffe4d : fman ffe4e-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac While for the latter I think we can put together a quick fix, for the former I'd like to take a bit of time to select the best fix, if one is really needed. So, please, let's split the two problems and first address the incorrect stack memory use. I have no idea how you can fix it without a (more correct this time) dummy region passed as parameter (and you don't want to use the first patch). But then it will be useless to do the call anyway, as it won't do any proper verification at all, so it could also be removed entirely, which begs the question, why do it at all in the first place (the devm_request_mem_region). I'm not an expert in that part of the code so feel free to correct me if I missed something. BR, Patrick H. Hi, Patrick, the DPAA entities are described in the device tree. Adding some hardcoding in the driver is not really the solution for this problem. And I'm not sure we have I'm not seeing any problem here, the offsets used by the fman driver were already there, I just reorganized them in 2 blocks. a clear problem statement to start with. Can you help me on that part? - The current call to __devm_request_region in fman_port.c is not correct. One way to fix this is to use devm_request_mem_region, however this requires that the main fman would not be reserving the whole region. This leads to the second problem: - Make sure the main fman driver is not reserving the whole region. Is that clearer like this ? Patrick H. Madalin
RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
> -Original Message- > From: Patrick Havelange > Sent: 08 December 2020 16:56 > To: Madalin Bucur ; David S. Miller > ; Jakub Kicinski ; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource > region reservation > > On 2020-12-03 16:47, Madalin Bucur wrote: > >> -Original Message- > >> From: Patrick Havelange > >> Sent: 03 December 2020 15:51 > >> To: Madalin Bucur ; David S. Miller > >> ; Jakub Kicinski ; > >> net...@vger.kernel.org; linux-kernel@vger.kernel.org > >> Cc: Patrick Havelange > >> Subject: [PATCH net 1/4] net: freescale/fman: Split the main resource > >> region reservation > >> > >> The main fman driver is only using some parts of the fman memory > >> region. > >> Split the reservation of the main region in 2, so that the other > >> regions that will be used by fman-port and fman-mac drivers can > >> be reserved properly and not be in conflict with the main fman > >> reservation. > >> > >> Signed-off-by: Patrick Havelange > > > > I think the problem you are trying to work on here is that the device > > tree entry that describes the FMan IP allots to the parent FMan device > the > > whole memory-mapped registers area, as described in the device datasheet. > > The smaller FMan building blocks (ports, MDIO controllers, etc.) are > > each using a nested subset of this memory-mapped registers area. > > While this hierarchical depiction of the hardware has not posed a > problem > > to date, it is possible to cause issues if both the FMan driver and any > > of the sub-blocks drivers are trying to exclusively reserve the > registers > > area. I'm assuming this is the problem you are trying to address here, > > besides the stack corruption issue. > > Yes exactly. > I did not add this behaviour (having a main region and subdrivers using > subregions), I'm just trying to correct what is already there. > For example: this is some content of /proc/iomem for one board I'm > working with, with the current existing code: > ffe40-ffe4fdfff : fman >ffe4e-ffe4e0fff : mac >ffe4e2000-ffe4e2fff : mac >ffe4e4000-ffe4e4fff : mac >ffe4e6000-ffe4e6fff : mac >ffe4e8000-ffe4e8fff : mac > > and now with my patches: > ffe40-ffe4fdfff : /soc@ffe00/fman@40 >ffe40-ffe480fff : fman >ffe488000-ffe488fff : fman-port >ffe489000-ffe489fff : fman-port >ffe48a000-ffe48afff : fman-port >ffe48b000-ffe48bfff : fman-port >ffe48c000-ffe48cfff : fman-port >ffe4a8000-ffe4a8fff : fman-port >ffe4a9000-ffe4a9fff : fman-port >ffe4aa000-ffe4aafff : fman-port >ffe4ab000-ffe4abfff : fman-port >ffe4ac000-ffe4acfff : fman-port >ffe4c-ffe4d : fman >ffe4e-ffe4e0fff : mac >ffe4e2000-ffe4e2fff : mac >ffe4e4000-ffe4e4fff : mac >ffe4e6000-ffe4e6fff : mac >ffe4e8000-ffe4e8fff : mac > > > While for the latter I think we can > > put together a quick fix, for the former I'd like to take a bit of time > > to select the best fix, if one is really needed. So, please, let's split > > the two problems and first address the incorrect stack memory use. > > I have no idea how you can fix it without a (more correct this time) > dummy region passed as parameter (and you don't want to use the first > patch). But then it will be useless to do the call anyway, as it won't > do any proper verification at all, so it could also be removed entirely, > which begs the question, why do it at all in the first place (the > devm_request_mem_region). > > I'm not an expert in that part of the code so feel free to correct me if > I missed something. > > BR, > > Patrick H. Hi, Patrick, the DPAA entities are described in the device tree. Adding some hardcoding in the driver is not really the solution for this problem. And I'm not sure we have a clear problem statement to start with. Can you help me on that part? Madalin
Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
On 2020-12-03 16:47, Madalin Bucur wrote: -Original Message- From: Patrick Havelange Sent: 03 December 2020 15:51 To: Madalin Bucur ; David S. Miller ; Jakub Kicinski ; net...@vger.kernel.org; linux-kernel@vger.kernel.org Cc: Patrick Havelange Subject: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation The main fman driver is only using some parts of the fman memory region. Split the reservation of the main region in 2, so that the other regions that will be used by fman-port and fman-mac drivers can be reserved properly and not be in conflict with the main fman reservation. Signed-off-by: Patrick Havelange I think the problem you are trying to work on here is that the device tree entry that describes the FMan IP allots to the parent FMan device the whole memory-mapped registers area, as described in the device datasheet. The smaller FMan building blocks (ports, MDIO controllers, etc.) are each using a nested subset of this memory-mapped registers area. While this hierarchical depiction of the hardware has not posed a problem to date, it is possible to cause issues if both the FMan driver and any of the sub-blocks drivers are trying to exclusively reserve the registers area. I'm assuming this is the problem you are trying to address here, besides the stack corruption issue. Yes exactly. I did not add this behaviour (having a main region and subdrivers using subregions), I'm just trying to correct what is already there. For example: this is some content of /proc/iomem for one board I'm working with, with the current existing code: ffe40-ffe4fdfff : fman ffe4e-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac and now with my patches: ffe40-ffe4fdfff : /soc@ffe00/fman@40 ffe40-ffe480fff : fman ffe488000-ffe488fff : fman-port ffe489000-ffe489fff : fman-port ffe48a000-ffe48afff : fman-port ffe48b000-ffe48bfff : fman-port ffe48c000-ffe48cfff : fman-port ffe4a8000-ffe4a8fff : fman-port ffe4a9000-ffe4a9fff : fman-port ffe4aa000-ffe4aafff : fman-port ffe4ab000-ffe4abfff : fman-port ffe4ac000-ffe4acfff : fman-port ffe4c-ffe4d : fman ffe4e-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac While for the latter I think we can put together a quick fix, for the former I'd like to take a bit of time to select the best fix, if one is really needed. So, please, let's split the two problems and first address the incorrect stack memory use. I have no idea how you can fix it without a (more correct this time) dummy region passed as parameter (and you don't want to use the first patch). But then it will be useless to do the call anyway, as it won't do any proper verification at all, so it could also be removed entirely, which begs the question, why do it at all in the first place (the devm_request_mem_region). I'm not an expert in that part of the code so feel free to correct me if I missed something. BR, Patrick H.
Re: [PATCH net 1/1] net: ipa: pass the correct size when freeing DMA memory
On Thu, 3 Dec 2020 17:28:16 -0600 Bjorn Andersson wrote:
> > When the coherent memory is freed in gsi_trans_pool_exit_dma(), we
> > are mistakenly passing the size of a single element in the pool
> > rather than the actual allocated size. Fix this bug.
> >
> > Fixes: 9dd441e4ed575 ("soc: qcom: ipa: GSI transactions")
> > Reported-by: Stephen Boyd
> > Tested-by: Sujit Kautkar
> > Signed-off-by: Alex Elder
>
> Reviewed-by: Bjorn Andersson
Applied, thanks!
Re: [PATCH net 1/1] net: ipa: pass the correct size when freeing DMA memory
On Thu 03 Dec 15:51 CST 2020, Alex Elder wrote:
> When the coherent memory is freed in gsi_trans_pool_exit_dma(), we
> are mistakenly passing the size of a single element in the pool
> rather than the actual allocated size. Fix this bug.
>
> Fixes: 9dd441e4ed575 ("soc: qcom: ipa: GSI transactions")
> Reported-by: Stephen Boyd
> Tested-by: Sujit Kautkar
> Signed-off-by: Alex Elder
Reviewed-by: Bjorn Andersson
Regards,
Bjorn
> ---
> drivers/net/ipa/gsi_trans.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
> index e8599bb948c08..6c3ed5b17b80c 100644
> --- a/drivers/net/ipa/gsi_trans.c
> +++ b/drivers/net/ipa/gsi_trans.c
> @@ -156,6 +156,9 @@ int gsi_trans_pool_init_dma(struct device *dev, struct
> gsi_trans_pool *pool,
> /* The allocator will give us a power-of-2 number of pages. But we
>* can't guarantee that, so request it. That way we won't waste any
>* memory that would be available beyond the required space.
> + *
> + * Note that gsi_trans_pool_exit_dma() assumes the total allocated
> + * size is exactly (count * size).
>*/
> total_size = get_order(total_size) << PAGE_SHIFT;
>
> @@ -175,7 +178,9 @@ int gsi_trans_pool_init_dma(struct device *dev, struct
> gsi_trans_pool *pool,
>
> void gsi_trans_pool_exit_dma(struct device *dev, struct gsi_trans_pool *pool)
> {
> - dma_free_coherent(dev, pool->size, pool->base, pool->addr);
> + size_t total_size = pool->count * pool->size;
> +
> + dma_free_coherent(dev, total_size, pool->base, pool->addr);
> memset(pool, 0, sizeof(*pool));
> }
>
> --
> 2.20.1
>
[PATCH net 1/1] net: ipa: pass the correct size when freeing DMA memory
When the coherent memory is freed in gsi_trans_pool_exit_dma(), we
are mistakenly passing the size of a single element in the pool
rather than the actual allocated size. Fix this bug.
Fixes: 9dd441e4ed575 ("soc: qcom: ipa: GSI transactions")
Reported-by: Stephen Boyd
Tested-by: Sujit Kautkar
Signed-off-by: Alex Elder
---
drivers/net/ipa/gsi_trans.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ipa/gsi_trans.c b/drivers/net/ipa/gsi_trans.c
index e8599bb948c08..6c3ed5b17b80c 100644
--- a/drivers/net/ipa/gsi_trans.c
+++ b/drivers/net/ipa/gsi_trans.c
@@ -156,6 +156,9 @@ int gsi_trans_pool_init_dma(struct device *dev, struct
gsi_trans_pool *pool,
/* The allocator will give us a power-of-2 number of pages. But we
* can't guarantee that, so request it. That way we won't waste any
* memory that would be available beyond the required space.
+*
+* Note that gsi_trans_pool_exit_dma() assumes the total allocated
+* size is exactly (count * size).
*/
total_size = get_order(total_size) << PAGE_SHIFT;
@@ -175,7 +178,9 @@ int gsi_trans_pool_init_dma(struct device *dev, struct
gsi_trans_pool *pool,
void gsi_trans_pool_exit_dma(struct device *dev, struct gsi_trans_pool *pool)
{
- dma_free_coherent(dev, pool->size, pool->base, pool->addr);
+ size_t total_size = pool->count * pool->size;
+
+ dma_free_coherent(dev, total_size, pool->base, pool->addr);
memset(pool, 0, sizeof(*pool));
}
--
2.20.1
RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
> -Original Message- > From: Patrick Havelange > Sent: 03 December 2020 15:51 > To: Madalin Bucur ; David S. Miller > ; Jakub Kicinski ; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Patrick Havelange > Subject: [PATCH net 1/4] net: freescale/fman: Split the main resource > region reservation > > The main fman driver is only using some parts of the fman memory > region. > Split the reservation of the main region in 2, so that the other > regions that will be used by fman-port and fman-mac drivers can > be reserved properly and not be in conflict with the main fman > reservation. > > Signed-off-by: Patrick Havelange I think the problem you are trying to work on here is that the device tree entry that describes the FMan IP allots to the parent FMan device the whole memory-mapped registers area, as described in the device datasheet. The smaller FMan building blocks (ports, MDIO controllers, etc.) are each using a nested subset of this memory-mapped registers area. While this hierarchical depiction of the hardware has not posed a problem to date, it is possible to cause issues if both the FMan driver and any of the sub-blocks drivers are trying to exclusively reserve the registers area. I'm assuming this is the problem you are trying to address here, besides the stack corruption issue. While for the latter I think we can put together a quick fix, for the former I'd like to take a bit of time to select the best fix, if one is really needed. So, please, let's split the two problems and first address the incorrect stack memory use. Regards, Madalin
[PATCH net 1/4] net: freescale/fman: Split the main resource region reservation
The main fman driver is only using some parts of the fman memory
region.
Split the reservation of the main region in 2, so that the other
regions that will be used by fman-port and fman-mac drivers can
be reserved properly and not be in conflict with the main fman
reservation.
Signed-off-by: Patrick Havelange
---
drivers/net/ethernet/freescale/fman/fman.c | 103 +
drivers/net/ethernet/freescale/fman/fman.h | 9 +-
2 files changed, 69 insertions(+), 43 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fman/fman.c
b/drivers/net/ethernet/freescale/fman/fman.c
index ce0a121580f6..2e85209d560d 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -58,12 +58,15 @@
/* Modules registers offsets */
#define BMI_OFFSET 0x0008
#define QMI_OFFSET 0x00080400
-#define KG_OFFSET 0x000C1000
-#define DMA_OFFSET 0x000C2000
-#define FPM_OFFSET 0x000C3000
-#define IMEM_OFFSET0x000C4000
-#define HWP_OFFSET 0x000C7000
-#define CGP_OFFSET 0x000DB000
+#define SIZE_REGION_0 0x00081000
+#define POL_OFFSET 0x000C
+#define KG_OFFSET_FROM_POL 0x1000
+#define DMA_OFFSET_FROM_POL0x2000
+#define FPM_OFFSET_FROM_POL0x3000
+#define IMEM_OFFSET_FROM_POL 0x4000
+#define HWP_OFFSET_FROM_POL0x7000
+#define CGP_OFFSET_FROM_POL0x0001B000
+#define SIZE_REGION_FROM_POL 0x0002
/* Exceptions bit map */
#define EX_DMA_BUS_ERROR 0x8000
@@ -1433,7 +1436,7 @@ static int clear_iram(struct fman *fman)
struct fman_iram_regs __iomem *iram;
int i, count;
- iram = fman->base_addr + IMEM_OFFSET;
+ iram = fman->base_addr_pol + IMEM_OFFSET_FROM_POL;
/* Enable the auto-increment */
iowrite32be(IRAM_IADD_AIE, >iadd);
@@ -1710,11 +1713,8 @@ static int set_num_of_open_dmas(struct fman *fman, u8
port_id,
static int fman_config(struct fman *fman)
{
- void __iomem *base_addr;
int err;
- base_addr = fman->dts_params.base_addr;
-
fman->state = kzalloc(sizeof(*fman->state), GFP_KERNEL);
if (!fman->state)
goto err_fm_state;
@@ -1740,13 +1740,14 @@ static int fman_config(struct fman *fman)
fman->state->res = fman->dts_params.res;
fman->exception_cb = fman_exceptions;
fman->bus_error_cb = fman_bus_error;
- fman->fpm_regs = base_addr + FPM_OFFSET;
- fman->bmi_regs = base_addr + BMI_OFFSET;
- fman->qmi_regs = base_addr + QMI_OFFSET;
- fman->dma_regs = base_addr + DMA_OFFSET;
- fman->hwp_regs = base_addr + HWP_OFFSET;
- fman->kg_regs = base_addr + KG_OFFSET;
- fman->base_addr = base_addr;
+ fman->fpm_regs = fman->dts_params.base_addr_pol + FPM_OFFSET_FROM_POL;
+ fman->bmi_regs = fman->dts_params.base_addr_0 + BMI_OFFSET;
+ fman->qmi_regs = fman->dts_params.base_addr_0 + QMI_OFFSET;
+ fman->dma_regs = fman->dts_params.base_addr_pol + DMA_OFFSET_FROM_POL;
+ fman->hwp_regs = fman->dts_params.base_addr_pol + HWP_OFFSET_FROM_POL;
+ fman->kg_regs = fman->dts_params.base_addr_pol + KG_OFFSET_FROM_POL;
+ fman->base_addr_0 = fman->dts_params.base_addr_0;
+ fman->base_addr_pol = fman->dts_params.base_addr_pol;
spin_lock_init(>spinlock);
fman_defconfig(fman->cfg);
@@ -1937,8 +1938,8 @@ static int fman_init(struct fman *fman)
fman->state->exceptions &= ~FMAN_EX_QMI_SINGLE_ECC;
/* clear CPG */
- memset_io((void __iomem *)(fman->base_addr + CGP_OFFSET), 0,
- fman->state->fm_port_num_of_cg);
+ memset_io((void __iomem *)(fman->base_addr_pol + CGP_OFFSET_FROM_POL),
+ 0, fman->state->fm_port_num_of_cg);
/* Save LIODN info before FMan reset
* Skipping non-existent port 0 (i = 1)
@@ -2717,13 +2718,11 @@ static struct fman *read_dts_node(struct
platform_device *of_dev)
{
struct fman *fman;
struct device_node *fm_node, *muram_node;
- struct resource *res;
+ struct resource *tmp_res, *main_res;
u32 val, range[2];
int err, irq;
struct clk *clk;
u32 clk_rate;
- phys_addr_t phys_base_addr;
- resource_size_t mem_size;
fman = kzalloc(sizeof(*fman), GFP_KERNEL);
if (!fman)
@@ -2740,34 +2739,31 @@ static struct fman *read_dts_node(struct
platform_device *of_dev)
fman->dts_params.id = (u8)val;
/* Get the FM interrupt */
- res = platform_get_resource(of_dev, IORESOURCE_IRQ, 0);
- if (!res) {
+ tmp_res = platform_get_resource(of_dev, IORESOURCE_IRQ, 0);
+ if (!tmp_res) {
dev_err(_dev->dev, "%s: Can't get FMan IRQ resource\n",
__func__);
goto fman_node_put;
}
- irq = res->start;
+ irq =
Re: [PATCH net 1/1] net: stmmac: Use rtnl_lock/unlock on netif_set_real_num_rx_queues() call
On Thu, 12 Nov 2020 22:49:48 +0800 Wong Vee Khee wrote:
> Fix an issue where dump stack is printed on suspend resume flow due to
> netif_set_real_num_rx_queues() is not called with rtnl_lock held().
>
> Fixes: 686cff3d7022 ("net: stmmac: Fix incorrect location to set
> real_num_rx|tx_queues")
> Reported-by: Christophe ROULLIER
> Tested-by: Christophe ROULLIER
> Cc: Alexandre TORGUE
> Reviewed-by: Ong Boon Leong
> Signed-off-by: Wong Vee Khee
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ba855465a2db..33e28004 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5278,7 +5278,10 @@ int stmmac_resume(struct device *dev)
>
> stmmac_clear_descriptors(priv);
>
> + rtnl_lock();
> stmmac_hw_setup(ndev, false);
> + rtnl_unlock();
> +
> stmmac_init_coalesce(priv);
> stmmac_set_rx_mode(ndev);
>
Doesn't look quite right. This is under the priv->lock which is
sometimes taken under rtnl_lock. So theoretically there could be
a deadlock.
You should probably take rtnl_lock() before priv->lock and release
it after. It's pretty common for drivers to hold rtnl_lock around
most of the resume method.
With larger context:
mutex_lock(>lock);
stmmac_reset_queues_param(priv);
stmmac_clear_descriptors(priv);
+ rtnl_lock();
stmmac_hw_setup(ndev, false);
+ rtnl_unlock();
+
stmmac_init_coalesce(priv);
stmmac_set_rx_mode(ndev);
stmmac_restore_hw_vlan_rx_fltr(priv, ndev, priv->hw);
stmmac_enable_all_queues(priv);
mutex_unlock(>lock);
[PATCH net 1/1] net: stmmac: Use rtnl_lock/unlock on netif_set_real_num_rx_queues() call
Fix an issue where dump stack is printed on suspend resume flow due to
netif_set_real_num_rx_queues() is not called with rtnl_lock held().
Fixes: 686cff3d7022 ("net: stmmac: Fix incorrect location to set
real_num_rx|tx_queues")
Reported-by: Christophe ROULLIER
Tested-by: Christophe ROULLIER
Cc: Alexandre TORGUE
Reviewed-by: Ong Boon Leong
Signed-off-by: Wong Vee Khee
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ba855465a2db..33e28004 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5278,7 +5278,10 @@ int stmmac_resume(struct device *dev)
stmmac_clear_descriptors(priv);
+ rtnl_lock();
stmmac_hw_setup(ndev, false);
+ rtnl_unlock();
+
stmmac_init_coalesce(priv);
stmmac_set_rx_mode(ndev);
--
2.17.0
Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
Hi Jakub, Matt, Jakub Kicinski 于2020年11月10日周二 上午6:20写道： > > On Mon, 9 Nov 2020 21:23:33 + (UTC) Matthieu Baerts wrote: > > 09 Nov 2020 21:57:05 Jakub Kicinski : > > > On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote: > > >> A small detail (I think): the Signed-off-by of the sender (Geliang) > > >> should be the last one in the list if I am not mistaken. > > >> But I guess this is not blocking. > > >> > > >> Reviewed-by: Matthieu Baerts > > > > > > I take it you'd like me to apply patch 1 directly to net? > > > > Sorry, I didn't know it was OK to apply only one patch of the series. > > Then yes, if you don't mind, please apply this patch :) > > Not really, I was just establishing ownership ;) > > Geliang Tang, please rebase on net and repost just the first patch. > It does not apply to net as is. v2 of this patch had been sent out. http://patchwork.ozlabs.org/project/netdev/patch/078a2ef5bdc4e3b2c25ef852461692001f426495.1604976945.git.geliangt...@gmail.com/ This patch should be applied to net-next, not -net. Since commit "mptcp: add a new sysctl add_addr_timeout" is not applied to -net yet. -Geliang
Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
On Mon, 9 Nov 2020 21:23:33 + (UTC) Matthieu Baerts wrote: > 09 Nov 2020 21:57:05 Jakub Kicinski : > > On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote: > >> A small detail (I think): the Signed-off-by of the sender (Geliang) > >> should be the last one in the list if I am not mistaken. > >> But I guess this is not blocking. > >> > >> Reviewed-by: Matthieu Baerts > > > > I take it you'd like me to apply patch 1 directly to net? > > Sorry, I didn't know it was OK to apply only one patch of the series. > Then yes, if you don't mind, please apply this patch :) Not really, I was just establishing ownership ;) Geliang Tang, please rebase on net and repost just the first patch. It does not apply to net as is.
Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
Hi Jakub, 09 Nov 2020 21:57:05 Jakub Kicinski : > On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote: >> A small detail (I think): the Signed-off-by of the sender (Geliang) >> should be the last one in the list if I am not mistaken. >> But I guess this is not blocking. >> >> Reviewed-by: Matthieu Baerts > > I take it you'd like me to apply patch 1 directly to net? Sorry, I didn't know it was OK to apply only one patch of the series. Then yes, if you don't mind, please apply this patch :) Cheers, Matt Tessares | Belgium | Hybrid Access Solutions www.tessares.net
Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote: > A small detail (I think): the Signed-off-by of the sender (Geliang) > should be the last one in the list if I am not mistaken. > But I guess this is not blocking. > > Reviewed-by: Matthieu Baerts I take it you'd like me to apply patch 1 directly to net? Or do you prefer to take it into mptcp tree first?
Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
On Mon, 9 Nov 2020 21:34:19 +0300 Dan Carpenter wrote: > Generally, I like them to be in chronological order. +1
Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
On Mon, Nov 09, 2020 at 05:28:54PM +0100, Matthieu Baerts wrote:
> Hi Geliang, Dan,
>
> On 09/11/2020 14:59, Geliang Tang wrote:
> > Fix the following Smatch complaint:
>
> Thanks for the report and the patch!
>
> > net/mptcp/pm_netlink.c:213 mptcp_pm_add_timer()
> > warn: variable dereferenced before check 'msk' (see line 208)
> >
> > net/mptcp/pm_netlink.c
> > 207 struct mptcp_sock *msk = entry->sock;
> > 208 struct sock *sk = (struct sock *)msk;
> > 209 struct net *net = sock_net(sk);
> > ^^
> > "msk" dereferenced here.
> >
> > 210
> > 211 pr_debug("msk=%p", msk);
> > 212
> > 213 if (!msk)
> >
> > Too late.
> >
> > 214 return;
> > 215
> >
> > Fixes: 93f323b9 ("mptcp: add a new sysctl add_addr_timeout")
> > Reported-by: Dan Carpenter
> > Signed-off-by: Geliang Tang
> > Reviewed-by: Dan Carpenter
>
> A small detail (I think): the Signed-off-by of the sender (Geliang) should
> be the last one in the list if I am not mistaken.
> But I guess this is not blocking.
Generally, I like them to be in chronological order. For other tags like
here it doesn't matter, but for signed-off-bys they only make sense in
chronological order.
regards,
dan carpenter
Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
Hi Geliang, Dan,
On 09/11/2020 14:59, Geliang Tang wrote:
Fix the following Smatch complaint:
Thanks for the report and the patch!
net/mptcp/pm_netlink.c:213 mptcp_pm_add_timer()
warn: variable dereferenced before check 'msk' (see line 208)
net/mptcp/pm_netlink.c
207 struct mptcp_sock *msk = entry->sock;
208 struct sock *sk = (struct sock *)msk;
209 struct net *net = sock_net(sk);
^^
"msk" dereferenced here.
210
211 pr_debug("msk=%p", msk);
212
213 if (!msk)
Too late.
214 return;
215
Fixes: 93f323b9 ("mptcp: add a new sysctl add_addr_timeout")
Reported-by: Dan Carpenter
Signed-off-by: Geliang Tang
Reviewed-by: Dan Carpenter
A small detail (I think): the Signed-off-by of the sender (Geliang)
should be the last one in the list if I am not mistaken.
But I guess this is not blocking.
Reviewed-by: Matthieu Baerts
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
[MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer
Fix the following Smatch complaint:
net/mptcp/pm_netlink.c:213 mptcp_pm_add_timer()
warn: variable dereferenced before check 'msk' (see line 208)
net/mptcp/pm_netlink.c
207 struct mptcp_sock *msk = entry->sock;
208 struct sock *sk = (struct sock *)msk;
209 struct net *net = sock_net(sk);
^^
"msk" dereferenced here.
210
211 pr_debug("msk=%p", msk);
212
213 if (!msk)
Too late.
214 return;
215
Fixes: 93f323b9 ("mptcp: add a new sysctl add_addr_timeout")
Reported-by: Dan Carpenter
Signed-off-by: Geliang Tang
Reviewed-by: Dan Carpenter
---
net/mptcp/pm_netlink.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 6180a8b39a3f..03f2c28f11f5 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -206,7 +206,6 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
struct mptcp_pm_add_entry *entry = from_timer(entry, timer, add_timer);
struct mptcp_sock *msk = entry->sock;
struct sock *sk = (struct sock *)msk;
- struct net *net = sock_net(sk);
pr_debug("msk=%p", msk);
@@ -235,7 +234,7 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
sk_reset_timer(sk, timer,
- jiffies + mptcp_get_add_addr_timeout(net));
+ jiffies +
mptcp_get_add_addr_timeout(sock_net(sk)));
spin_unlock_bh(>pm.lock);
--
2.26.2
Re: [PATCH net 1/1] ptp: idt82p33: add adjphase support
On Fri, 30 Oct 2020 13:55:32 -0400 min.li...@renesas.com wrote: > From: Min Li > > Add idt82p33_adjwritephase() to support PHC write phase mode. > > Signed-off-by: Min Li Doesn't build on 32bit. make[2]: *** Deleting file 'Module.symvers' ERROR: modpost: "__divdi3" [drivers/ptp/ptp_idt82p33.ko] undefined! make[2]: *** [Module.symvers] Error 1 make[1]: *** [modules] Error 2 make: *** [__sub-make] Error 2
[PATCH net 1/1] ptp: idt82p33: add adjphase support
From: Min Li
Add idt82p33_adjwritephase() to support PHC write phase mode.
Signed-off-by: Min Li
---
drivers/ptp/ptp_idt82p33.c | 307 +++--
drivers/ptp/ptp_idt82p33.h | 3 +
2 files changed, 243 insertions(+), 67 deletions(-)
diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
index 179f6c4..3d91aee 100644
--- a/drivers/ptp/ptp_idt82p33.c
+++ b/drivers/ptp/ptp_idt82p33.c
@@ -33,6 +33,9 @@ module_param(phase_snap_threshold, uint, 0);
MODULE_PARM_DESC(phase_snap_threshold,
"threshold (15ns by default) below which adjtime would ignore");
+static char *firmware;
+module_param(firmware, charp, 0);
+
static void idt82p33_byte_array_to_timespec(struct timespec64 *ts,
u8 buf[TOD_BYTE_COUNT])
{
@@ -77,15 +80,15 @@ static void idt82p33_timespec_to_byte_array(struct
timespec64 const *ts,
}
}
-static int idt82p33_xfer(struct idt82p33 *idt82p33,
-unsigned char regaddr,
-unsigned char *buf,
-unsigned int count,
-int write)
+static int idt82p33_xfer_read(struct idt82p33 *idt82p33,
+ unsigned char regaddr,
+ unsigned char *buf,
+ unsigned int count)
{
struct i2c_client *client = idt82p33->client;
struct i2c_msg msg[2];
int cnt;
+ char *fmt = "i2c_transfer failed at %d in %s, at addr: %04X!\n";
msg[0].addr = client->addr;
msg[0].flags = 0;
@@ -93,13 +96,17 @@ static int idt82p33_xfer(struct idt82p33 *idt82p33,
msg[0].buf =
msg[1].addr = client->addr;
- msg[1].flags = write ? 0 : I2C_M_RD;
+ msg[1].flags = I2C_M_RD;
msg[1].len = count;
msg[1].buf = buf;
cnt = i2c_transfer(client->adapter, msg, 2);
if (cnt < 0) {
- dev_err(>dev, "i2c_transfer returned %d\n", cnt);
+ dev_err(>dev,
+ fmt,
+ __LINE__,
+ __func__,
+ (u8) regaddr);
return cnt;
} else if (cnt != 2) {
dev_err(>dev,
@@ -109,6 +116,37 @@ static int idt82p33_xfer(struct idt82p33 *idt82p33,
return 0;
}
+static int idt82p33_xfer_write(struct idt82p33 *idt82p33,
+ u8 regaddr,
+ u8 *buf,
+ u16 count)
+{
+ struct i2c_client *client = idt82p33->client;
+ /* we add 1 byte for device register */
+ u8 msg[IDT82P33_MAX_WRITE_COUNT + 1];
+ int cnt;
+ char *fmt = "i2c_master_send failed at %d in %s, at addr: %04X!\n";
+
+ if (count > IDT82P33_MAX_WRITE_COUNT)
+ return -EINVAL;
+
+ msg[0] = regaddr;
+ memcpy([1], buf, count);
+
+ cnt = i2c_master_send(client, msg, count + 1);
+
+ if (cnt < 0) {
+ dev_err(>dev,
+ fmt,
+ __LINE__,
+ __func__,
+ regaddr);
+ return cnt;
+ }
+
+ return 0;
+}
+
static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
{
int err;
@@ -116,7 +154,7 @@ static int idt82p33_page_offset(struct idt82p33 *idt82p33,
unsigned char val)
if (idt82p33->page_offset == val)
return 0;
- err = idt82p33_xfer(idt82p33, PAGE_ADDR, , sizeof(val), 1);
+ err = idt82p33_xfer_write(idt82p33, PAGE_ADDR, , sizeof(val));
if (err)
dev_err(>client->dev,
"failed to set page offset %d\n", val);
@@ -137,11 +175,12 @@ static int idt82p33_rdwr(struct idt82p33 *idt82p33,
unsigned int regaddr,
err = idt82p33_page_offset(idt82p33, page);
if (err)
- goto out;
+ return err;
- err = idt82p33_xfer(idt82p33, offset, buf, count, write);
-out:
- return err;
+ if (write)
+ return idt82p33_xfer_write(idt82p33, offset, buf, count);
+
+ return idt82p33_xfer_read(idt82p33, offset, buf, count);
}
static int idt82p33_read(struct idt82p33 *idt82p33, unsigned int regaddr,
@@ -448,8 +487,13 @@ static int idt82p33_measure_tod_write_overhead(struct
idt82p33_channel *channel)
err = idt82p33_measure_settime_gettime_gap_overhead(channel, _ns);
- if (err)
+ if (err) {
+ dev_err(>client->dev,
+ "Failed at line %d in func %s!\n",
+ __LINE__,
+ __func__);
return err;
+ }
err = idt82p33_measure_one_byte_write_overhead(channel,
_byte_write_ns);
@@ -518,12 +562,10 @@ static int idt82p33_sync_tod(struct idt82p33_channel
*channel, bool enable)
u8
Re: [PATCH net 1/1] stmmac: intel: Fix kernel panic on pci probe
On Thu, 29 Oct 2020 17:32:28 +0800 Wong Vee Khee wrote:
> The commit "stmmac: intel: Adding ref clock 1us tic for LPI cntr"
> introduced a regression which leads to the kernel panic duing loading
> of the dwmac_intel module.
>
> Move the code block after pci resources is obtained.
>
> Fixes: b4c5f83ae3f3 ("stmmac: intel: Adding ref clock 1us tic for LPI cntr")
> Cc: Voon Weifeng
> Signed-off-by: Wong Vee Khee
Applied, thanks!
[PATCH net 1/1] stmmac: intel: Fix kernel panic on pci probe
The commit "stmmac: intel: Adding ref clock 1us tic for LPI cntr"
introduced a regression which leads to the kernel panic duing loading
of the dwmac_intel module.
Move the code block after pci resources is obtained.
Fixes: b4c5f83ae3f3 ("stmmac: intel: Adding ref clock 1us tic for LPI cntr")
Cc: Voon Weifeng
Signed-off-by: Wong Vee Khee
---
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index b6e5e3e36b63..81ee0a071b4e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -625,13 +625,6 @@ static int intel_eth_pci_probe(struct pci_dev *pdev,
if (ret)
return ret;
- if (plat->eee_usecs_rate > 0) {
- u32 tx_lpi_usec;
-
- tx_lpi_usec = (plat->eee_usecs_rate / 100) - 1;
- writel(tx_lpi_usec, res.addr + GMAC_1US_TIC_COUNTER);
- }
-
ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
if (ret < 0)
return ret;
@@ -641,6 +634,13 @@ static int intel_eth_pci_probe(struct pci_dev *pdev,
res.wol_irq = pci_irq_vector(pdev, 0);
res.irq = pci_irq_vector(pdev, 0);
+ if (plat->eee_usecs_rate > 0) {
+ u32 tx_lpi_usec;
+
+ tx_lpi_usec = (plat->eee_usecs_rate / 100) - 1;
+ writel(tx_lpi_usec, res.addr + GMAC_1US_TIC_COUNTER);
+ }
+
ret = stmmac_dvr_probe(>dev, plat, );
if (ret) {
pci_free_irq_vectors(pdev);
--
2.17.0
[PATCH net 1/5] net: ipa: assign proper packet context base
At the end of ipa_mem_setup() we write the local packet processing
context base register to tell it where the processing context memory
is. But we are writing the wrong value.
The value written turns out to be the offset of the modem header
memory region (assigned earlier in the function). Fix this bug.
Fixes: ba764c4dad7bd ("soc: qcom: ipa: clocking, interrupts, and memory")
Signed-off-by: Alex Elder
---
drivers/net/ipa/ipa_mem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index 2d45c444a67fa..ecfd1f91fce3b 100644
--- a/drivers/net/ipa/ipa_mem.c
+++ b/drivers/net/ipa/ipa_mem.c
@@ -89,7 +89,7 @@ int ipa_mem_setup(struct ipa *ipa)
gsi_trans_commit_wait(trans);
/* Tell the hardware where the processing context area is located */
- iowrite32(ipa->mem_offset + offset,
+ iowrite32(ipa->mem_offset + ipa->mem[IPA_MEM_MODEM_PROC_CTX].offset,
ipa->reg_virt + IPA_REG_LOCAL_PKT_PROC_CNTXT_BASE_OFFSET);
return 0;
--
2.20.1
Re: [PATCH net 1/4] ptp: ptp_idt82p33: update to support adjphase
Hi!
> +static int idt82p33_adjwritephase(struct ptp_clock_info *ptp, s32
> +offsetNs) {
adj_write_phase?
> + struct idt82p33_channel *channel =
> + container_of(ptp, struct idt82p33_channel, caps);
> + struct idt82p33 *idt82p33 = channel->idt82p33;
> + s64 offsetInFs;
> + s64 offsetRegVal;
> + u8 val[4] = {0};
> + int err;
> +
> + offsetInFs = (s64)(-offsetNs) * 100;
> +
> + if (offsetInFs > WRITE_PHASE_OFFSET_LIMIT)
> + offsetInFs = WRITE_PHASE_OFFSET_LIMIT;
> + else if (offsetInFs < -WRITE_PHASE_OFFSET_LIMIT)
> + offsetInFs = -WRITE_PHASE_OFFSET_LIMIT;
I'm sure we have macro for this.
> + /* Convert from phaseOffsetInFs to register value */
> + offsetRegVal = ((offsetInFs * 1000) / IDT_T0DPLL_PHASE_RESOL);
> +
> + val[0] = offsetRegVal & 0xFF;
> + val[1] = (offsetRegVal >> 8) & 0xFF;
> + val[2] = (offsetRegVal >> 16) & 0xFF;
> + val[3] = (offsetRegVal >> 24) & 0x1F;
> + val[3] |= PH_OFFSET_EN;
ThisIsReally far away from usual coding style.
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
signature.asc
Description: Digital signature
Re: [PATCH net 1/4] ptp: ptp_idt82p33: update to support adjphase
On Thu, Oct 15, 2020 at 07:30:38PM +, Min Li wrote: > When you have time, can you take a look at this change? Thanks Min, I think your series was posted during a time when net-next was closed. Please report the series. Thanks, Richard
