All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
@ 2020-12-18 17:38 Florian Fainelli
  2020-12-18 20:24 ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-12-18 17:38 UTC (permalink / raw)
  To: netdev
  Cc: andrew, olteanv, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

The driver is already allocating receive buffers of 2KiB and the
Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.

Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 0fdd19d99d99..b1ae9eb8f247 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
 			 NETIF_F_HW_VLAN_CTAG_TX;
 	dev->hw_features |= dev->features;
 	dev->vlan_features |= dev->features;
+	dev->max_mtu = UMAC_MAX_MTU_SIZE;
 
 	/* Request the WOL interrupt and advertise suspend if available */
 	priv->wol_irq_disabled = 1;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 17:38 [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE Florian Fainelli
@ 2020-12-18 20:24 ` Vladimir Oltean
  2020-12-18 20:30   ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2020-12-18 20:24 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

Hi Florian,

On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
> The driver is already allocating receive buffers of 2KiB and the
> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
> 
> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 0fdd19d99d99..b1ae9eb8f247 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
>  			 NETIF_F_HW_VLAN_CTAG_TX;
>  	dev->hw_features |= dev->features;
>  	dev->vlan_features |= dev->features;
> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
>  
>  	/* Request the WOL interrupt and advertise suspend if available */
>  	priv->wol_irq_disabled = 1;
> -- 
> 2.25.1
> 

Do you want to treat the SYSTEMPORT Lite differently?

	/* Set maximum frame length */
	if (!priv->is_lite)
		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
	else
		gib_set_pad_extension(priv);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 20:24 ` Vladimir Oltean
@ 2020-12-18 20:30   ` Florian Fainelli
  2020-12-18 20:52     ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-12-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On 12/18/20 12:24 PM, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
>> The driver is already allocating receive buffers of 2KiB and the
>> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
>>
>> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 0fdd19d99d99..b1ae9eb8f247 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
>>  			 NETIF_F_HW_VLAN_CTAG_TX;
>>  	dev->hw_features |= dev->features;
>>  	dev->vlan_features |= dev->features;
>> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
>>  
>>  	/* Request the WOL interrupt and advertise suspend if available */
>>  	priv->wol_irq_disabled = 1;
>> -- 
>> 2.25.1
>>
> 
> Do you want to treat the SYSTEMPORT Lite differently?
> 
> 	/* Set maximum frame length */
> 	if (!priv->is_lite)
> 		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
> 	else
> 		gib_set_pad_extension(priv);

SYSTEMPORT Lite does not actually validate the frame length, so setting
a maximum number to the buffer size we allocate could work, but I don't
see a reason to differentiate the two types of MACs here.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 20:30   ` Florian Fainelli
@ 2020-12-18 20:52     ` Vladimir Oltean
  2020-12-18 20:54       ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2020-12-18 20:52 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On Fri, Dec 18, 2020 at 12:30:20PM -0800, Florian Fainelli wrote:
> On 12/18/20 12:24 PM, Vladimir Oltean wrote:
> > Hi Florian,
> > 
> > On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
> >> The driver is already allocating receive buffers of 2KiB and the
> >> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
> >>
> >> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> >> index 0fdd19d99d99..b1ae9eb8f247 100644
> >> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> >> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> >> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
> >>  			 NETIF_F_HW_VLAN_CTAG_TX;
> >>  	dev->hw_features |= dev->features;
> >>  	dev->vlan_features |= dev->features;
> >> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
> >>  
> >>  	/* Request the WOL interrupt and advertise suspend if available */
> >>  	priv->wol_irq_disabled = 1;
> >> -- 
> >> 2.25.1
> >>
> > 
> > Do you want to treat the SYSTEMPORT Lite differently?
> > 
> > 	/* Set maximum frame length */
> > 	if (!priv->is_lite)
> > 		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
> > 	else
> > 		gib_set_pad_extension(priv);
> 
> SYSTEMPORT Lite does not actually validate the frame length, so setting
> a maximum number to the buffer size we allocate could work, but I don't
> see a reason to differentiate the two types of MACs here.

And if the Lite doesn't validate the frame length, then shouldn't it
report a max_mtu equal to the max_mtu of the attached DSA switch, plus
the Broadcom tag length? Doesn't the b53 driver support jumbo frames?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 20:52     ` Vladimir Oltean
@ 2020-12-18 20:54       ` Florian Fainelli
  2020-12-18 21:02         ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-12-18 20:54 UTC (permalink / raw)
  To: Vladimir Oltean, Florian Fainelli
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On 12/18/20 12:52 PM, Vladimir Oltean wrote:
> On Fri, Dec 18, 2020 at 12:30:20PM -0800, Florian Fainelli wrote:
>> On 12/18/20 12:24 PM, Vladimir Oltean wrote:
>>> Hi Florian,
>>>
>>> On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
>>>> The driver is already allocating receive buffers of 2KiB and the
>>>> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
>>>>
>>>> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>>>> index 0fdd19d99d99..b1ae9eb8f247 100644
>>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>>>> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
>>>>  			 NETIF_F_HW_VLAN_CTAG_TX;
>>>>  	dev->hw_features |= dev->features;
>>>>  	dev->vlan_features |= dev->features;
>>>> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
>>>>  
>>>>  	/* Request the WOL interrupt and advertise suspend if available */
>>>>  	priv->wol_irq_disabled = 1;
>>>> -- 
>>>> 2.25.1
>>>>
>>>
>>> Do you want to treat the SYSTEMPORT Lite differently?
>>>
>>> 	/* Set maximum frame length */
>>> 	if (!priv->is_lite)
>>> 		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
>>> 	else
>>> 		gib_set_pad_extension(priv);
>>
>> SYSTEMPORT Lite does not actually validate the frame length, so setting
>> a maximum number to the buffer size we allocate could work, but I don't
>> see a reason to differentiate the two types of MACs here.
> 
> And if the Lite doesn't validate the frame length, then shouldn't it
> report a max_mtu equal to the max_mtu of the attached DSA switch, plus
> the Broadcom tag length? Doesn't the b53 driver support jumbo frames?

And how would I do that without create a horrible layering violation in
either the systemport driver or DSA? Yes the b53 driver supports jumbo
frames.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 20:54       ` Florian Fainelli
@ 2020-12-18 21:02         ` Vladimir Oltean
  2020-12-18 21:08           ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2020-12-18 21:02 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On Fri, Dec 18, 2020 at 12:54:33PM -0800, Florian Fainelli wrote:
> On 12/18/20 12:52 PM, Vladimir Oltean wrote:
> > On Fri, Dec 18, 2020 at 12:30:20PM -0800, Florian Fainelli wrote:
> >> On 12/18/20 12:24 PM, Vladimir Oltean wrote:
> >>> Hi Florian,
> >>>
> >>> On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
> >>>> The driver is already allocating receive buffers of 2KiB and the
> >>>> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
> >>>>
> >>>> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >>>> ---
> >>>>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> >>>> index 0fdd19d99d99..b1ae9eb8f247 100644
> >>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> >>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> >>>> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
> >>>>  			 NETIF_F_HW_VLAN_CTAG_TX;
> >>>>  	dev->hw_features |= dev->features;
> >>>>  	dev->vlan_features |= dev->features;
> >>>> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
> >>>>  
> >>>>  	/* Request the WOL interrupt and advertise suspend if available */
> >>>>  	priv->wol_irq_disabled = 1;
> >>>> -- 
> >>>> 2.25.1
> >>>>
> >>>
> >>> Do you want to treat the SYSTEMPORT Lite differently?
> >>>
> >>> 	/* Set maximum frame length */
> >>> 	if (!priv->is_lite)
> >>> 		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
> >>> 	else
> >>> 		gib_set_pad_extension(priv);
> >>
> >> SYSTEMPORT Lite does not actually validate the frame length, so setting
> >> a maximum number to the buffer size we allocate could work, but I don't
> >> see a reason to differentiate the two types of MACs here.
> > 
> > And if the Lite doesn't validate the frame length, then shouldn't it
> > report a max_mtu equal to the max_mtu of the attached DSA switch, plus
> > the Broadcom tag length? Doesn't the b53 driver support jumbo frames?
> 
> And how would I do that without create a horrible layering violation in
> either the systemport driver or DSA? Yes the b53 driver supports jumbo
> frames.

Sorry, I don't understand where is the layering violation (maybe it doesn't
help me either that I'm not familiar with Broadcom architectures).

Is the SYSTEMPORT Lite always used as a DSA master, or could it also be
used standalone? What would be the issue with hardcoding a max_mtu value
which is large enough for b53 to use jumbo frames?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 21:02         ` Vladimir Oltean
@ 2020-12-18 21:08           ` Florian Fainelli
  2020-12-18 21:14             ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-12-18 21:08 UTC (permalink / raw)
  To: Vladimir Oltean, Florian Fainelli
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On 12/18/20 1:02 PM, Vladimir Oltean wrote:
> On Fri, Dec 18, 2020 at 12:54:33PM -0800, Florian Fainelli wrote:
>> On 12/18/20 12:52 PM, Vladimir Oltean wrote:
>>> On Fri, Dec 18, 2020 at 12:30:20PM -0800, Florian Fainelli wrote:
>>>> On 12/18/20 12:24 PM, Vladimir Oltean wrote:
>>>>> Hi Florian,
>>>>>
>>>>> On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
>>>>>> The driver is already allocating receive buffers of 2KiB and the
>>>>>> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
>>>>>>
>>>>>> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>> ---
>>>>>>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>>>>>> index 0fdd19d99d99..b1ae9eb8f247 100644
>>>>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>>>>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>>>>>> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
>>>>>>  			 NETIF_F_HW_VLAN_CTAG_TX;
>>>>>>  	dev->hw_features |= dev->features;
>>>>>>  	dev->vlan_features |= dev->features;
>>>>>> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
>>>>>>  
>>>>>>  	/* Request the WOL interrupt and advertise suspend if available */
>>>>>>  	priv->wol_irq_disabled = 1;
>>>>>> -- 
>>>>>> 2.25.1
>>>>>>
>>>>>
>>>>> Do you want to treat the SYSTEMPORT Lite differently?
>>>>>
>>>>> 	/* Set maximum frame length */
>>>>> 	if (!priv->is_lite)
>>>>> 		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
>>>>> 	else
>>>>> 		gib_set_pad_extension(priv);
>>>>
>>>> SYSTEMPORT Lite does not actually validate the frame length, so setting
>>>> a maximum number to the buffer size we allocate could work, but I don't
>>>> see a reason to differentiate the two types of MACs here.
>>>
>>> And if the Lite doesn't validate the frame length, then shouldn't it
>>> report a max_mtu equal to the max_mtu of the attached DSA switch, plus
>>> the Broadcom tag length? Doesn't the b53 driver support jumbo frames?
>>
>> And how would I do that without create a horrible layering violation in
>> either the systemport driver or DSA? Yes the b53 driver supports jumbo
>> frames.
> 
> Sorry, I don't understand where is the layering violation (maybe it doesn't
> help me either that I'm not familiar with Broadcom architectures).
> 
> Is the SYSTEMPORT Lite always used as a DSA master, or could it also be
> used standalone? What would be the issue with hardcoding a max_mtu value
> which is large enough for b53 to use jumbo frames?

SYSTEMPORT Lite is always used as a DSA master AFAICT given its GMII
Integration Block (GIB) was specifically designed with another MAC and
particularly that of a switch on the other side.

The layering violation I am concerned with is that we do not know ahead
of time which b53 switch we are going to be interfaced with, and they
have various limitations on the sizes they support. Right now b53 only
concerns itself with returning JMS_MAX_SIZE, but I am fairly positive
this needs fixing given the existing switches supported by the driver.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 21:08           ` Florian Fainelli
@ 2020-12-18 21:14             ` Vladimir Oltean
  2020-12-18 21:17               ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2020-12-18 21:14 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On Fri, Dec 18, 2020 at 01:08:58PM -0800, Florian Fainelli wrote:
> On 12/18/20 1:02 PM, Vladimir Oltean wrote:
> > On Fri, Dec 18, 2020 at 12:54:33PM -0800, Florian Fainelli wrote:
> >> On 12/18/20 12:52 PM, Vladimir Oltean wrote:
> >>> On Fri, Dec 18, 2020 at 12:30:20PM -0800, Florian Fainelli wrote:
> >>>> On 12/18/20 12:24 PM, Vladimir Oltean wrote:
> >>>>> Hi Florian,
> >>>>>
> >>>>> On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
> >>>>>> The driver is already allocating receive buffers of 2KiB and the
> >>>>>> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
> >>>>>>
> >>>>>> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
> >>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >>>>>> ---
> >>>>>>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
> >>>>>>  1 file changed, 1 insertion(+)
> >>>>>>
> >>>>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> >>>>>> index 0fdd19d99d99..b1ae9eb8f247 100644
> >>>>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> >>>>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> >>>>>> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
> >>>>>>  			 NETIF_F_HW_VLAN_CTAG_TX;
> >>>>>>  	dev->hw_features |= dev->features;
> >>>>>>  	dev->vlan_features |= dev->features;
> >>>>>> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
> >>>>>>  
> >>>>>>  	/* Request the WOL interrupt and advertise suspend if available */
> >>>>>>  	priv->wol_irq_disabled = 1;
> >>>>>> -- 
> >>>>>> 2.25.1
> >>>>>>
> >>>>>
> >>>>> Do you want to treat the SYSTEMPORT Lite differently?
> >>>>>
> >>>>> 	/* Set maximum frame length */
> >>>>> 	if (!priv->is_lite)
> >>>>> 		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
> >>>>> 	else
> >>>>> 		gib_set_pad_extension(priv);
> >>>>
> >>>> SYSTEMPORT Lite does not actually validate the frame length, so setting
> >>>> a maximum number to the buffer size we allocate could work, but I don't
> >>>> see a reason to differentiate the two types of MACs here.
> >>>
> >>> And if the Lite doesn't validate the frame length, then shouldn't it
> >>> report a max_mtu equal to the max_mtu of the attached DSA switch, plus
> >>> the Broadcom tag length? Doesn't the b53 driver support jumbo frames?
> >>
> >> And how would I do that without create a horrible layering violation in
> >> either the systemport driver or DSA? Yes the b53 driver supports jumbo
> >> frames.
> > 
> > Sorry, I don't understand where is the layering violation (maybe it doesn't
> > help me either that I'm not familiar with Broadcom architectures).
> > 
> > Is the SYSTEMPORT Lite always used as a DSA master, or could it also be
> > used standalone? What would be the issue with hardcoding a max_mtu value
> > which is large enough for b53 to use jumbo frames?
> 
> SYSTEMPORT Lite is always used as a DSA master AFAICT given its GMII
> Integration Block (GIB) was specifically designed with another MAC and
> particularly that of a switch on the other side.
> 
> The layering violation I am concerned with is that we do not know ahead
> of time which b53 switch we are going to be interfaced with, and they
> have various limitations on the sizes they support. Right now b53 only
> concerns itself with returning JMS_MAX_SIZE, but I am fairly positive
> this needs fixing given the existing switches supported by the driver.

Maybe we don't need to over-engineer this. As long as you report a large
enough max_mtu in the SYSTEMPORT Lite driver to accomodate for all
possible revisions of embedded switches, and the max_mtu of the switch
itself is still accurate and representative of the switch revision limits,
I think that's good enough.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 21:14             ` Vladimir Oltean
@ 2020-12-18 21:17               ` Florian Fainelli
  2020-12-21 21:49                 ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-12-18 21:17 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On 12/18/20 1:14 PM, Vladimir Oltean wrote:
> On Fri, Dec 18, 2020 at 01:08:58PM -0800, Florian Fainelli wrote:
>> On 12/18/20 1:02 PM, Vladimir Oltean wrote:
>>> On Fri, Dec 18, 2020 at 12:54:33PM -0800, Florian Fainelli wrote:
>>>> On 12/18/20 12:52 PM, Vladimir Oltean wrote:
>>>>> On Fri, Dec 18, 2020 at 12:30:20PM -0800, Florian Fainelli wrote:
>>>>>> On 12/18/20 12:24 PM, Vladimir Oltean wrote:
>>>>>>> Hi Florian,
>>>>>>>
>>>>>>> On Fri, Dec 18, 2020 at 09:38:43AM -0800, Florian Fainelli wrote:
>>>>>>>> The driver is already allocating receive buffers of 2KiB and the
>>>>>>>> Ethernet MAC is configured to accept frames up to UMAC_MAX_MTU_SIZE.
>>>>>>>>
>>>>>>>> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
>>>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>>>> ---
>>>>>>>>  drivers/net/ethernet/broadcom/bcmsysport.c | 1 +
>>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>>>>>>>> index 0fdd19d99d99..b1ae9eb8f247 100644
>>>>>>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>>>>>>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>>>>>>>> @@ -2577,6 +2577,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
>>>>>>>>  			 NETIF_F_HW_VLAN_CTAG_TX;
>>>>>>>>  	dev->hw_features |= dev->features;
>>>>>>>>  	dev->vlan_features |= dev->features;
>>>>>>>> +	dev->max_mtu = UMAC_MAX_MTU_SIZE;
>>>>>>>>  
>>>>>>>>  	/* Request the WOL interrupt and advertise suspend if available */
>>>>>>>>  	priv->wol_irq_disabled = 1;
>>>>>>>> -- 
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>>
>>>>>>> Do you want to treat the SYSTEMPORT Lite differently?
>>>>>>>
>>>>>>> 	/* Set maximum frame length */
>>>>>>> 	if (!priv->is_lite)
>>>>>>> 		umac_writel(priv, UMAC_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
>>>>>>> 	else
>>>>>>> 		gib_set_pad_extension(priv);
>>>>>>
>>>>>> SYSTEMPORT Lite does not actually validate the frame length, so setting
>>>>>> a maximum number to the buffer size we allocate could work, but I don't
>>>>>> see a reason to differentiate the two types of MACs here.
>>>>>
>>>>> And if the Lite doesn't validate the frame length, then shouldn't it
>>>>> report a max_mtu equal to the max_mtu of the attached DSA switch, plus
>>>>> the Broadcom tag length? Doesn't the b53 driver support jumbo frames?
>>>>
>>>> And how would I do that without create a horrible layering violation in
>>>> either the systemport driver or DSA? Yes the b53 driver supports jumbo
>>>> frames.
>>>
>>> Sorry, I don't understand where is the layering violation (maybe it doesn't
>>> help me either that I'm not familiar with Broadcom architectures).
>>>
>>> Is the SYSTEMPORT Lite always used as a DSA master, or could it also be
>>> used standalone? What would be the issue with hardcoding a max_mtu value
>>> which is large enough for b53 to use jumbo frames?
>>
>> SYSTEMPORT Lite is always used as a DSA master AFAICT given its GMII
>> Integration Block (GIB) was specifically designed with another MAC and
>> particularly that of a switch on the other side.
>>
>> The layering violation I am concerned with is that we do not know ahead
>> of time which b53 switch we are going to be interfaced with, and they
>> have various limitations on the sizes they support. Right now b53 only
>> concerns itself with returning JMS_MAX_SIZE, but I am fairly positive
>> this needs fixing given the existing switches supported by the driver.
> 
> Maybe we don't need to over-engineer this. As long as you report a large
> enough max_mtu in the SYSTEMPORT Lite driver to accomodate for all
> possible revisions of embedded switches, and the max_mtu of the switch
> itself is still accurate and representative of the switch revision limits,
> I think that's good enough.

I suppose that is fair, v2 coming, thanks!
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-18 21:17               ` Florian Fainelli
@ 2020-12-21 21:49                 ` Florian Fainelli
  2020-12-21 22:25                   ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-12-21 21:49 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list



On 12/18/2020 1:17 PM, Florian Fainelli wrote:
>>>>>>> SYSTEMPORT Lite does not actually validate the frame length, so setting
>>>>>>> a maximum number to the buffer size we allocate could work, but I don't
>>>>>>> see a reason to differentiate the two types of MACs here.
>>>>>>
>>>>>> And if the Lite doesn't validate the frame length, then shouldn't it
>>>>>> report a max_mtu equal to the max_mtu of the attached DSA switch, plus
>>>>>> the Broadcom tag length? Doesn't the b53 driver support jumbo frames?
>>>>>
>>>>> And how would I do that without create a horrible layering violation in
>>>>> either the systemport driver or DSA? Yes the b53 driver supports jumbo
>>>>> frames.
>>>>
>>>> Sorry, I don't understand where is the layering violation (maybe it doesn't
>>>> help me either that I'm not familiar with Broadcom architectures).
>>>>
>>>> Is the SYSTEMPORT Lite always used as a DSA master, or could it also be
>>>> used standalone? What would be the issue with hardcoding a max_mtu value
>>>> which is large enough for b53 to use jumbo frames?
>>>
>>> SYSTEMPORT Lite is always used as a DSA master AFAICT given its GMII
>>> Integration Block (GIB) was specifically designed with another MAC and
>>> particularly that of a switch on the other side.
>>>
>>> The layering violation I am concerned with is that we do not know ahead
>>> of time which b53 switch we are going to be interfaced with, and they
>>> have various limitations on the sizes they support. Right now b53 only
>>> concerns itself with returning JMS_MAX_SIZE, but I am fairly positive
>>> this needs fixing given the existing switches supported by the driver.
>>
>> Maybe we don't need to over-engineer this. As long as you report a large
>> enough max_mtu in the SYSTEMPORT Lite driver to accomodate for all
>> possible revisions of embedded switches, and the max_mtu of the switch
>> itself is still accurate and representative of the switch revision limits,
>> I think that's good enough.
> 
> I suppose that is fair, v2 coming, thanks!

I was going to issue a v2 for this patch, but given that we don't
allocate buffers larger than 2KiB and there is really no need to
implement ndo_change_mtu(), is there really a point not to use
UMAC_MAX_MTU_SIZE for both variants of the SYSTEMPORT MAC?
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-21 21:49                 ` Florian Fainelli
@ 2020-12-21 22:25                   ` Vladimir Oltean
  2020-12-21 22:55                     ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2020-12-21 22:25 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list

On Mon, Dec 21, 2020 at 01:49:03PM -0800, Florian Fainelli wrote:
> On 12/18/2020 1:17 PM, Florian Fainelli wrote:
> >>>>>>> SYSTEMPORT Lite does not actually validate the frame length, so setting
> >>>>>>> a maximum number to the buffer size we allocate could work, but I don't
> >>>>>>> see a reason to differentiate the two types of MACs here.
> >>>>>>
> >>>>>> And if the Lite doesn't validate the frame length, then shouldn't it
> >>>>>> report a max_mtu equal to the max_mtu of the attached DSA switch, plus
> >>>>>> the Broadcom tag length? Doesn't the b53 driver support jumbo frames?
> >>>>>
> >>>>> And how would I do that without create a horrible layering violation in
> >>>>> either the systemport driver or DSA? Yes the b53 driver supports jumbo
> >>>>> frames.
> >>>>
> >>>> Sorry, I don't understand where is the layering violation (maybe it doesn't
> >>>> help me either that I'm not familiar with Broadcom architectures).
> >>>>
> >>>> Is the SYSTEMPORT Lite always used as a DSA master, or could it also be
> >>>> used standalone? What would be the issue with hardcoding a max_mtu value
> >>>> which is large enough for b53 to use jumbo frames?
> >>>
> >>> SYSTEMPORT Lite is always used as a DSA master AFAICT given its GMII
> >>> Integration Block (GIB) was specifically designed with another MAC and
> >>> particularly that of a switch on the other side.
> >>>
> >>> The layering violation I am concerned with is that we do not know ahead
> >>> of time which b53 switch we are going to be interfaced with, and they
> >>> have various limitations on the sizes they support. Right now b53 only
> >>> concerns itself with returning JMS_MAX_SIZE, but I am fairly positive
> >>> this needs fixing given the existing switches supported by the driver.
> >>
> >> Maybe we don't need to over-engineer this. As long as you report a large
> >> enough max_mtu in the SYSTEMPORT Lite driver to accomodate for all
> >> possible revisions of embedded switches, and the max_mtu of the switch
> >> itself is still accurate and representative of the switch revision limits,
> >> I think that's good enough.
> >
> > I suppose that is fair, v2 coming, thanks!
>
> I was going to issue a v2 for this patch, but given that we don't
> allocate buffers larger than 2KiB and there is really no need to
> implement ndo_change_mtu(), is there really a point not to use
> UMAC_MAX_MTU_SIZE for both variants of the SYSTEMPORT MAC?

After your first reply that "the Lite doesn't validate the frame length", I was
under the impression that it is sufficient to declare a larger max_mtu such as
JMS_MAX_SIZE and 9K jumbo frames would just work. But with the current buffer
allocation in bcm_sysport_rx_refill it clearly wouldn't. A stupid confusion
really. So yeah, sorry for having you resend a v2 with no change.
If it helps you could add to the patch:

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Thanks again for explaining.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE
  2020-12-21 22:25                   ` Vladimir Oltean
@ 2020-12-21 22:55                     ` Florian Fainelli
  0 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2020-12-21 22:55 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, andrew, David S. Miller, Jakub Kicinski,
	Murali Krishna Policharla, Vladimir Oltean,
	open list:BROADCOM SYSTEMPORT ETHERNET DRIVER, open list



On 12/21/2020 2:25 PM, Vladimir Oltean wrote:
> On Mon, Dec 21, 2020 at 01:49:03PM -0800, Florian Fainelli wrote:
>> On 12/18/2020 1:17 PM, Florian Fainelli wrote:
>>>>>>>>> SYSTEMPORT Lite does not actually validate the frame length, so setting
>>>>>>>>> a maximum number to the buffer size we allocate could work, but I don't
>>>>>>>>> see a reason to differentiate the two types of MACs here.
>>>>>>>>
>>>>>>>> And if the Lite doesn't validate the frame length, then shouldn't it
>>>>>>>> report a max_mtu equal to the max_mtu of the attached DSA switch, plus
>>>>>>>> the Broadcom tag length? Doesn't the b53 driver support jumbo frames?
>>>>>>>
>>>>>>> And how would I do that without create a horrible layering violation in
>>>>>>> either the systemport driver or DSA? Yes the b53 driver supports jumbo
>>>>>>> frames.
>>>>>>
>>>>>> Sorry, I don't understand where is the layering violation (maybe it doesn't
>>>>>> help me either that I'm not familiar with Broadcom architectures).
>>>>>>
>>>>>> Is the SYSTEMPORT Lite always used as a DSA master, or could it also be
>>>>>> used standalone? What would be the issue with hardcoding a max_mtu value
>>>>>> which is large enough for b53 to use jumbo frames?
>>>>>
>>>>> SYSTEMPORT Lite is always used as a DSA master AFAICT given its GMII
>>>>> Integration Block (GIB) was specifically designed with another MAC and
>>>>> particularly that of a switch on the other side.
>>>>>
>>>>> The layering violation I am concerned with is that we do not know ahead
>>>>> of time which b53 switch we are going to be interfaced with, and they
>>>>> have various limitations on the sizes they support. Right now b53 only
>>>>> concerns itself with returning JMS_MAX_SIZE, but I am fairly positive
>>>>> this needs fixing given the existing switches supported by the driver.
>>>>
>>>> Maybe we don't need to over-engineer this. As long as you report a large
>>>> enough max_mtu in the SYSTEMPORT Lite driver to accomodate for all
>>>> possible revisions of embedded switches, and the max_mtu of the switch
>>>> itself is still accurate and representative of the switch revision limits,
>>>> I think that's good enough.
>>>
>>> I suppose that is fair, v2 coming, thanks!
>>
>> I was going to issue a v2 for this patch, but given that we don't
>> allocate buffers larger than 2KiB and there is really no need to
>> implement ndo_change_mtu(), is there really a point not to use
>> UMAC_MAX_MTU_SIZE for both variants of the SYSTEMPORT MAC?
> 
> After your first reply that "the Lite doesn't validate the frame length", I was
> under the impression that it is sufficient to declare a larger max_mtu such as
> JMS_MAX_SIZE and 9K jumbo frames would just work. But with the current buffer
> allocation in bcm_sysport_rx_refill it clearly wouldn't. A stupid confusion
> really. So yeah, sorry for having you resend a v2 with no change.
> If it helps you could add to the patch:
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> 
> Thanks again for explaining.

No worries, Jakub, David, do you need me to resend or can you pick it up
from patchwork?
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-12-21 22:56 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-18 17:38 [PATCH net] net: systemport: set dev->max_mtu to UMAC_MAX_MTU_SIZE Florian Fainelli
2020-12-18 20:24 ` Vladimir Oltean
2020-12-18 20:30   ` Florian Fainelli
2020-12-18 20:52     ` Vladimir Oltean
2020-12-18 20:54       ` Florian Fainelli
2020-12-18 21:02         ` Vladimir Oltean
2020-12-18 21:08           ` Florian Fainelli
2020-12-18 21:14             ` Vladimir Oltean
2020-12-18 21:17               ` Florian Fainelli
2020-12-21 21:49                 ` Florian Fainelli
2020-12-21 22:25                   ` Vladimir Oltean
2020-12-21 22:55                     ` Florian Fainelli

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.