Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
Hi,
On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
> Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> as below. Fix by maintaining a reference to each PHY node in slave
> struct instead of a single reference in the priv struct which was
> overwritten by the 2nd PHY.
David, Is it possible to drop prev version of this patch from linux-next
- it breaks boot on many TI boards with -next.
>
> [ 17.870933] Unable to handle kernel NULL pointer dereference at virtual
> address 0180
> [ 17.879557] pgd = dc8bc000
> [ 17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> [ 17.889213] Internal error: Oops: 17 [#1] ARM
> [ 17.893838] Modules linked in:
> [ 17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted
> 4.5.0-ge463dfb-dirty #11
> [ 17.904947] Hardware name: Cambrionix whippet
> [ 17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> [ 17.915339] PC is at phy_attached_print+0x18/0x8c
> [ 17.920339] LR is at phy_attached_info+0x14/0x18
> [ 17.925247] pc : []lr : []psr: 600f0113
> [ 17.925247] sp : dc969cf8 ip : dc969d28 fp : dc969d18
> [ 17.937425] r10: dda7a400 r9 : r8 :
> [ 17.942971] r7 : 0001 r6 : ddb00480 r5 : ddb8cb34 r4 :
> [ 17.949898] r3 : c0954cc0 r2 : c09562b0 r1 : r0 :
> [ 17.956829] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment
> none
> [ 17.964401] Control: 10c5387d Table: 9c8bc019 DAC: 0051
> [ 17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> [ 17.977059] Stack: (0xdc969cf8 to 0xdc96a000)
[...]
> [ 18.323956] [] (inet_ioctl) from []
> (sock_ioctl+0x15c/0x2d8)
> [ 18.331829] [] (sock_ioctl) from []
> (do_vfs_ioctl+0x98/0x8d0)
> [ 18.339765] r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> [ 18.345822] [] (do_vfs_ioctl) from []
> (SyS_ioctl+0x74/0x84)
> [ 18.353573] r10: r9:0011 r8:beaeda20 r7:8914 r6:dc8ab4c0
> r5:dc8ab4c0
> [ 18.361924] r4:
> [ 18.364653] [] (SyS_ioctl) from []
> (ret_fast_syscall+0x0/0x3c)
> [ 18.372682] r9:dc968000 r8:c00165e8 r7:0036 r6:0002 r5:0011
> r4:
> [ 18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> [ 18.387580] ---[ end trace c80529466223f3f3 ]---
^ Could you make it shorter and drop timestamps, pls?
>
> Signed-off-by: Andrew Goodbody
> ---
>
> v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so it
> has data->slaves initialised first which is needed to calculate size
>
> drivers/net/ethernet/ti/cpsw.c | 30 +++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 42fdfd4..e62909c 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -349,6 +349,7 @@ struct cpsw_slave {
> struct cpsw_slave_data *data;
> struct phy_device *phy;
> struct net_device *ndev;
> + struct device_node *phy_node;
> u32 port_vlan;
> u32 open_stat;
> };
> @@ -367,7 +368,6 @@ struct cpsw_priv {
> spinlock_t lock;
> struct platform_device *pdev;
> struct net_device *ndev;
> - struct device_node *phy_node;
> struct napi_struct napi_rx;
> struct napi_struct napi_tx;
> struct device *dev;
> @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave,
> struct cpsw_priv *priv)
> cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
> 1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
>
> - if (priv->phy_node)
> - slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
> + if (slave->phy_node)
> + slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
>&cpsw_adjust_link, 0, slave->data->phy_if);
> else
> slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> struct device_node *node = pdev->dev.of_node;
> struct device_node *slave_node;
> struct cpsw_platform_data *data = &priv->data;
> - int i = 0, ret;
> + int i, ret;
> u32 prop;
>
> if (!node)
> @@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> }
> data->slaves = prop;
>
> + priv->slaves = devm_kzalloc(&pdev->dev,
> + sizeof(struct cpsw_slave) * data->slaves,
> + GFP_KERNEL);
> + if (!priv->slaves)
> + return -ENOMEM;
> + for (i = 0; i < data->slaves; i++)
> + priv->slaves[i].slave_num = i;
> +
> if (of_prope
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
On Tue, 19 Apr 2016 17:41:07 +0300
Grygorii Strashko wrote:
> Hi,
>
> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
> > Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> > as below. Fix by maintaining a reference to each PHY node in slave
> > struct instead of a single reference in the priv struct which was
> > overwritten by the 2nd PHY.
>
> David, Is it possible to drop prev version of this patch from linux-next
> - it breaks boot on many TI boards with -next.
>
>
> >
> > [ 17.870933] Unable to handle kernel NULL pointer dereference at virtual
> > address 0180
> > [ 17.879557] pgd = dc8bc000
> > [ 17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> > [ 17.889213] Internal error: Oops: 17 [#1] ARM
> > [ 17.893838] Modules linked in:
> > [ 17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted
> > 4.5.0-ge463dfb-dirty #11
> > [ 17.904947] Hardware name: Cambrionix whippet
> > [ 17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> > [ 17.915339] PC is at phy_attached_print+0x18/0x8c
> > [ 17.920339] LR is at phy_attached_info+0x14/0x18
> > [ 17.925247] pc : []lr : []psr: 600f0113
> > [ 17.925247] sp : dc969cf8 ip : dc969d28 fp : dc969d18
> > [ 17.937425] r10: dda7a400 r9 : r8 :
> > [ 17.942971] r7 : 0001 r6 : ddb00480 r5 : ddb8cb34 r4 :
> > [ 17.949898] r3 : c0954cc0 r2 : c09562b0 r1 : r0 :
> > [ 17.956829] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment
> > none
> > [ 17.964401] Control: 10c5387d Table: 9c8bc019 DAC: 0051
> > [ 17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> > [ 17.977059] Stack: (0xdc969cf8 to 0xdc96a000)
>
> [...]
>
> > [ 18.323956] [] (inet_ioctl) from []
> > (sock_ioctl+0x15c/0x2d8)
> > [ 18.331829] [] (sock_ioctl) from []
> > (do_vfs_ioctl+0x98/0x8d0)
> > [ 18.339765] r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> > [ 18.345822] [] (do_vfs_ioctl) from []
> > (SyS_ioctl+0x74/0x84)
> > [ 18.353573] r10: r9:0011 r8:beaeda20 r7:8914
> > r6:dc8ab4c0 r5:dc8ab4c0
> > [ 18.361924] r4:
> > [ 18.364653] [] (SyS_ioctl) from []
> > (ret_fast_syscall+0x0/0x3c)
> > [ 18.372682] r9:dc968000 r8:c00165e8 r7:0036 r6:0002 r5:0011
> > r4:
> > [ 18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> > [ 18.387580] ---[ end trace c80529466223f3f3 ]---
>
> ^ Could you make it shorter and drop timestamps, pls?
>
> >
> > Signed-off-by: Andrew Goodbody
> > ---
> >
> > v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so
> > it
> > has data->slaves initialised first which is needed to calculate size
> >
> > drivers/net/ethernet/ti/cpsw.c | 30 +++---
> > 1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 42fdfd4..e62909c 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -349,6 +349,7 @@ struct cpsw_slave {
> > struct cpsw_slave_data *data;
> > struct phy_device *phy;
> > struct net_device *ndev;
> > + struct device_node *phy_node;
> > u32 port_vlan;
> > u32 open_stat;
> > };
> > @@ -367,7 +368,6 @@ struct cpsw_priv {
> > spinlock_t lock;
> > struct platform_device *pdev;
> > struct net_device *ndev;
> > - struct device_node *phy_node;
> > struct napi_struct napi_rx;
> > struct napi_struct napi_tx;
> > struct device *dev;
> > @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave,
> > struct cpsw_priv *priv)
> > cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
> >1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
> >
> > - if (priv->phy_node)
> > - slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
> > + if (slave->phy_node)
> > + slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
> > &cpsw_adjust_link, 0, slave->data->phy_if);
> > else
> > slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
> > @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > struct device_node *node = pdev->dev.of_node;
> > struct device_node *slave_node;
> > struct cpsw_platform_data *data = &priv->data;
> > - int i = 0, ret;
> > + int i, ret;
> > u32 prop;
> >
> > if (!node)
> > @@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> > }
> > data->slaves = prop;
> >
> > + priv->slaves = devm_kzalloc(&pdev->dev,
> > + sizeof(struct cpsw_slave) * d
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
On 04/19/2016 06:01 PM, David Rivshin (Allworx) wrote:
> On Tue, 19 Apr 2016 17:41:07 +0300
> Grygorii Strashko wrote:
>
>> Hi,
>>
>> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
>>> Adding a 2nd PHY to cpsw results in a NULL pointer dereference
>>> as below. Fix by maintaining a reference to each PHY node in slave
>>> struct instead of a single reference in the priv struct which was
>>> overwritten by the 2nd PHY.
>>
>> David, Is it possible to drop prev version of this patch from linux-next
>> - it breaks boot on many TI boards with -next.
>>
>>
>>>
>>> [ 17.870933] Unable to handle kernel NULL pointer dereference at virtual
>>> address 0180
>>> [ 17.879557] pgd = dc8bc000
>>> [ 17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
>>> [ 17.889213] Internal error: Oops: 17 [#1] ARM
>>> [ 17.893838] Modules linked in:
>>> [ 17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted
>>> 4.5.0-ge463dfb-dirty #11
>>> [ 17.904947] Hardware name: Cambrionix whippet
>>> [ 17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
>>> [ 17.915339] PC is at phy_attached_print+0x18/0x8c
>>> [ 17.920339] LR is at phy_attached_info+0x14/0x18
>>> [ 17.925247] pc : []lr : []psr: 600f0113
>>> [ 17.925247] sp : dc969cf8 ip : dc969d28 fp : dc969d18
>>> [ 17.937425] r10: dda7a400 r9 : r8 :
>>> [ 17.942971] r7 : 0001 r6 : ddb00480 r5 : ddb8cb34 r4 :
>>> [ 17.949898] r3 : c0954cc0 r2 : c09562b0 r1 : r0 :
>>> [ 17.956829] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment
>>> none
>>> [ 17.964401] Control: 10c5387d Table: 9c8bc019 DAC: 0051
>>> [ 17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
>>> [ 17.977059] Stack: (0xdc969cf8 to 0xdc96a000)
>>
>> [...]
>>
>>> [ 18.323956] [] (inet_ioctl) from []
>>> (sock_ioctl+0x15c/0x2d8)
>>> [ 18.331829] [] (sock_ioctl) from []
>>> (do_vfs_ioctl+0x98/0x8d0)
>>> [ 18.339765] r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
>>> [ 18.345822] [] (do_vfs_ioctl) from []
>>> (SyS_ioctl+0x74/0x84)
>>> [ 18.353573] r10: r9:0011 r8:beaeda20 r7:8914
>>> r6:dc8ab4c0 r5:dc8ab4c0
>>> [ 18.361924] r4:
>>> [ 18.364653] [] (SyS_ioctl) from []
>>> (ret_fast_syscall+0x0/0x3c)
>>> [ 18.372682] r9:dc968000 r8:c00165e8 r7:0036 r6:0002 r5:0011
>>> r4:
>>> [ 18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
>>> [ 18.387580] ---[ end trace c80529466223f3f3 ]---
>>
>> ^ Could you make it shorter and drop timestamps, pls?
>>
>>>
>>> Signed-off-by: Andrew Goodbody
>>> ---
>>>
>>> v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so
>>> it
>>>has data->slaves initialised first which is needed to calculate size
>>>
>>>drivers/net/ethernet/ti/cpsw.c | 30 +++---
>>>1 file changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>>> index 42fdfd4..e62909c 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -349,6 +349,7 @@ struct cpsw_slave {
>>> struct cpsw_slave_data *data;
>>> struct phy_device *phy;
>>> struct net_device *ndev;
>>> + struct device_node *phy_node;
>>> u32 port_vlan;
>>> u32 open_stat;
>>>};
>>> @@ -367,7 +368,6 @@ struct cpsw_priv {
>>> spinlock_t lock;
>>> struct platform_device *pdev;
>>> struct net_device *ndev;
>>> - struct device_node *phy_node;
>>> struct napi_struct napi_rx;
>>> struct napi_struct napi_tx;
>>> struct device *dev;
>>> @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave,
>>> struct cpsw_priv *priv)
>>> cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
>>>1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
>>>
>>> - if (priv->phy_node)
>>> - slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
>>> + if (slave->phy_node)
>>> + slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
>>> &cpsw_adjust_link, 0, slave->data->phy_if);
>>> else
>>> slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
>>> @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
>>> struct device_node *node = pdev->dev.of_node;
>>> struct device_node *slave_node;
>>> struct cpsw_platform_data *data = &priv->data;
>>> - int i = 0, ret;
>>> + int i, ret;
>>> u32 prop;
>>>
>>> if (!node)
>>> @@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
>>> }
>>> data->slaves = prop;
>>>
>>> + priv->slaves = devm_kzalloc(&pdev->dev,
>>
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
From: Grygorii Strashko Date: Tue, 19 Apr 2016 17:41:07 +0300 > David, Is it possible to drop prev version of this patch from linux-next > - it breaks boot on many TI boards with -next. It doesn't work that way, I cannot "drop" patches. One has to send me a fix to the existing patch, or a revert.
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
On Tue, 19 Apr 2016 18:44:41 +0300
Grygorii Strashko wrote:
> On 04/19/2016 06:01 PM, David Rivshin (Allworx) wrote:
> > On Tue, 19 Apr 2016 17:41:07 +0300
> > Grygorii Strashko wrote:
> >
> >> Hi,
> >>
> >> On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
> >>> Adding a 2nd PHY to cpsw results in a NULL pointer dereference
> >>> as below. Fix by maintaining a reference to each PHY node in slave
> >>> struct instead of a single reference in the priv struct which was
> >>> overwritten by the 2nd PHY.
> >>
> >> David, Is it possible to drop prev version of this patch from linux-next
> >> - it breaks boot on many TI boards with -next.
> >>
> >>
> >>>
> >>> [ 17.870933] Unable to handle kernel NULL pointer dereference at
> >>> virtual address 0180
> >>> [ 17.879557] pgd = dc8bc000
> >>> [ 17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
> >>> [ 17.889213] Internal error: Oops: 17 [#1] ARM
> >>> [ 17.893838] Modules linked in:
> >>> [ 17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted
> >>> 4.5.0-ge463dfb-dirty #11
> >>> [ 17.904947] Hardware name: Cambrionix whippet
> >>> [ 17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
> >>> [ 17.915339] PC is at phy_attached_print+0x18/0x8c
> >>> [ 17.920339] LR is at phy_attached_info+0x14/0x18
> >>> [ 17.925247] pc : []lr : []psr: 600f0113
> >>> [ 17.925247] sp : dc969cf8 ip : dc969d28 fp : dc969d18
> >>> [ 17.937425] r10: dda7a400 r9 : r8 :
> >>> [ 17.942971] r7 : 0001 r6 : ddb00480 r5 : ddb8cb34 r4 :
> >>> [ 17.949898] r3 : c0954cc0 r2 : c09562b0 r1 : r0 :
> >>> [ 17.956829] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM
> >>> Segment none
> >>> [ 17.964401] Control: 10c5387d Table: 9c8bc019 DAC: 0051
> >>> [ 17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
> >>> [ 17.977059] Stack: (0xdc969cf8 to 0xdc96a000)
> >>
> >> [...]
> >>
> >>> [ 18.323956] [] (inet_ioctl) from []
> >>> (sock_ioctl+0x15c/0x2d8)
> >>> [ 18.331829] [] (sock_ioctl) from []
> >>> (do_vfs_ioctl+0x98/0x8d0)
> >>> [ 18.339765] r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
> >>> [ 18.345822] [] (do_vfs_ioctl) from []
> >>> (SyS_ioctl+0x74/0x84)
> >>> [ 18.353573] r10: r9:0011 r8:beaeda20 r7:8914
> >>> r6:dc8ab4c0 r5:dc8ab4c0
> >>> [ 18.361924] r4:
> >>> [ 18.364653] [] (SyS_ioctl) from []
> >>> (ret_fast_syscall+0x0/0x3c)
> >>> [ 18.372682] r9:dc968000 r8:c00165e8 r7:0036 r6:0002
> >>> r5:0011 r4:
> >>> [ 18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
> >>> [ 18.387580] ---[ end trace c80529466223f3f3 ]---
> >>
> >> ^ Could you make it shorter and drop timestamps, pls?
> >>
> >>>
> >>> Signed-off-by: Andrew Goodbody
> >>> ---
> >>>
> >>> v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt
> >>> so it
> >>>has data->slaves initialised first which is needed to calculate
> >>> size
> >>>
> >>>drivers/net/ethernet/ti/cpsw.c | 30 +++---
> >>>1 file changed, 15 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/ti/cpsw.c
> >>> b/drivers/net/ethernet/ti/cpsw.c
> >>> index 42fdfd4..e62909c 100644
> >>> --- a/drivers/net/ethernet/ti/cpsw.c
> >>> +++ b/drivers/net/ethernet/ti/cpsw.c
> >>> @@ -349,6 +349,7 @@ struct cpsw_slave {
> >>> struct cpsw_slave_data *data;
> >>> struct phy_device *phy;
> >>> struct net_device *ndev;
> >>> + struct device_node *phy_node;
> >>> u32 port_vlan;
> >>> u32 open_stat;
> >>>};
> >>> @@ -367,7 +368,6 @@ struct cpsw_priv {
> >>> spinlock_t lock;
> >>> struct platform_device *pdev;
> >>> struct net_device *ndev;
> >>> - struct device_node *phy_node;
> >>> struct napi_struct napi_rx;
> >>> struct napi_struct napi_tx;
> >>> struct device *dev;
> >>> @@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave
> >>> *slave, struct cpsw_priv *priv)
> >>> cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
> >>> 1 << slave_port, 0, 0,
> >>> ALE_MCAST_FWD_2);
> >>>
> >>> - if (priv->phy_node)
> >>> - slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
> >>> + if (slave->phy_node)
> >>> + slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
> >>>&cpsw_adjust_link, 0,
> >>> slave->data->phy_if);
> >>> else
> >>> slave->phy = phy_connect(priv->ndev,
> >>> slave->data->phy_id,
> >>> @@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
> >>
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
On 19/04/16 10:14, David Rivshin (Allworx) wrote: >> Ah Ok. There are no user of cpsw_platform_data outside of net/ethernet/ti/, >> so yes, looks like your patch 1 does exactly what's needed. > > Given that the v1 of Andrew's patch is already in Dave's net tree, and > would obviously have many conflicts with mine, how should I proceed? > Since you already requested Dave revert that patch, should I just wait > for that to happen and then resubmit my series? > > Dave, Does that sound good to you? At some point, it would be good for the cpsw driver to be moved to the DSA subsystem, a lot of these tiny little details, like how to attach the PHY to the internal, external, fixed MDIO bus have been solved already using standard DT bindings. This leaves you with the fun part: how to program your switch such that it works in response to the networking stack sollicitations (bridge, VLAN, ethtool etc.). -- Florian
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
On 04/19/2016 08:14 PM, David Rivshin (Allworx) wrote:
On Tue, 19 Apr 2016 18:44:41 +0300
Grygorii Strashko wrote:
On 04/19/2016 06:01 PM, David Rivshin (Allworx) wrote:
On Tue, 19 Apr 2016 17:41:07 +0300
Grygorii Strashko wrote:
Hi,
On 04/19/2016 04:56 PM, Andrew Goodbody wrote:
Adding a 2nd PHY to cpsw results in a NULL pointer dereference
as below. Fix by maintaining a reference to each PHY node in slave
struct instead of a single reference in the priv struct which was
overwritten by the 2nd PHY.
David, Is it possible to drop prev version of this patch from linux-next
- it breaks boot on many TI boards with -next.
[ 17.870933] Unable to handle kernel NULL pointer dereference at virtual
address 0180
[ 17.879557] pgd = dc8bc000
[ 17.882514] [0180] *pgd=9c882831, *pte=, *ppte=
[ 17.889213] Internal error: Oops: 17 [#1] ARM
[ 17.893838] Modules linked in:
[ 17.897102] CPU: 0 PID: 1657 Comm: connmand Not tainted 4.5.0-ge463dfb-dirty
#11
[ 17.904947] Hardware name: Cambrionix whippet
[ 17.909576] task: dc859240 ti: dc968000 task.ti: dc968000
[ 17.915339] PC is at phy_attached_print+0x18/0x8c
[ 17.920339] LR is at phy_attached_info+0x14/0x18
[ 17.925247] pc : []lr : []psr: 600f0113
[ 17.925247] sp : dc969cf8 ip : dc969d28 fp : dc969d18
[ 17.937425] r10: dda7a400 r9 : r8 :
[ 17.942971] r7 : 0001 r6 : ddb00480 r5 : ddb8cb34 r4 :
[ 17.949898] r3 : c0954cc0 r2 : c09562b0 r1 : r0 :
[ 17.956829] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 17.964401] Control: 10c5387d Table: 9c8bc019 DAC: 0051
[ 17.970500] Process connmand (pid: 1657, stack limit = 0xdc968210)
[ 17.977059] Stack: (0xdc969cf8 to 0xdc96a000)
[...]
[ 18.323956] [] (inet_ioctl) from []
(sock_ioctl+0x15c/0x2d8)
[ 18.331829] [] (sock_ioctl) from []
(do_vfs_ioctl+0x98/0x8d0)
[ 18.339765] r7:8914 r6:dc8ab4c0 r5:dd257ae0 r4:beaeda20
[ 18.345822] [] (do_vfs_ioctl) from []
(SyS_ioctl+0x74/0x84)
[ 18.353573] r10: r9:0011 r8:beaeda20 r7:8914 r6:dc8ab4c0
r5:dc8ab4c0
[ 18.361924] r4:
[ 18.364653] [] (SyS_ioctl) from []
(ret_fast_syscall+0x0/0x3c)
[ 18.372682] r9:dc968000 r8:c00165e8 r7:0036 r6:0002 r5:0011
r4:
[ 18.380960] Code: e92dd810 e24cb010 e24dd010 e59b4004 (e5902180)
[ 18.387580] ---[ end trace c80529466223f3f3 ]---
^ Could you make it shorter and drop timestamps, pls?
Signed-off-by: Andrew Goodbody
---
v2 - Move allocation of memory for priv->slaves to inside cpsw_probe_dt so it
has data->slaves initialised first which is needed to calculate size
drivers/net/ethernet/ti/cpsw.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 42fdfd4..e62909c 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -349,6 +349,7 @@ struct cpsw_slave {
struct cpsw_slave_data *data;
struct phy_device *phy;
struct net_device *ndev;
+ struct device_node *phy_node;
u32 port_vlan;
u32 open_stat;
};
@@ -367,7 +368,6 @@ struct cpsw_priv {
spinlock_t lock;
struct platform_device *pdev;
struct net_device *ndev;
- struct device_node *phy_node;
struct napi_struct napi_rx;
struct napi_struct napi_tx;
struct device *dev;
@@ -1148,8 +1148,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave,
struct cpsw_priv *priv)
cpsw_ale_add_mcast(priv->ale, priv->ndev->broadcast,
1 << slave_port, 0, 0, ALE_MCAST_FWD_2);
- if (priv->phy_node)
- slave->phy = of_phy_connect(priv->ndev, priv->phy_node,
+ if (slave->phy_node)
+ slave->phy = of_phy_connect(priv->ndev, slave->phy_node,
&cpsw_adjust_link, 0, slave->data->phy_if);
else
slave->phy = phy_connect(priv->ndev, slave->data->phy_id,
@@ -1946,7 +1946,7 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
struct device_node *node = pdev->dev.of_node;
struct device_node *slave_node;
struct cpsw_platform_data *data = &priv->data;
- int i = 0, ret;
+ int i, ret;
u32 prop;
if (!node)
@@ -1958,6 +1958,14 @@ static int cpsw_probe_dt(struct cpsw_priv *priv,
}
data->slaves = prop;
+ priv->slaves = devm_kzalloc(&pdev->dev,
+ sizeof(struct cpsw_slave) * data->slaves,
+ GFP_KERNEL);
+ if (!priv->slaves)
+ return -ENOMEM;
+
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
From: Grygorii Strashko Date: Tue, 19 Apr 2016 21:44:09 +0300 > May be you can send revert + your patch 1 (only fix for this issue). > > Dave, Does that sound good to you? Sure.
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
On Tue, 19 Apr 2016 18:43:39 -0400 (EDT) David Miller wrote: > From: Grygorii Strashko > Date: Tue, 19 Apr 2016 21:44:09 +0300 > > > May be you can send revert + your patch 1 (only fix for this issue). > > > > Dave, Does that sound good to you? > > Sure. OK, I will hopefully have that ready tomorrow evening. Anything in particular I should mention in the revert commit message? I didn't find a discussion on it, other than the earlier statement that "it breaks boot on many TI boards".
Re: [PATCH v2 1/1] drivers: net: cpsw: Prevent NUll pointer dereference with two PHYs
On 04/20/2016 02:38 AM, David Rivshin (Allworx) wrote: > On Tue, 19 Apr 2016 18:43:39 -0400 (EDT) > David Miller wrote: > >> From: Grygorii Strashko >> Date: Tue, 19 Apr 2016 21:44:09 +0300 >> >>> May be you can send revert + your patch 1 (only fix for this issue). >>> >>> Dave, Does that sound good to you? >> >> Sure. > > OK, I will hopefully have that ready tomorrow evening. > > Anything in particular I should mention in the revert commit message? > I didn't find a discussion on it, other than the earlier statement that > "it breaks boot on many TI boards". > Such kind of issues are tracked automatically now for ARM through https://kernelci.org Example: https://storage.kernelci.org/next/next-20160419/arm-multi_v7_defconfig+CONFIG_EFI=y/lab-cambridge/boot-am335x-boneblack.txt And We're trying to auto check it internally also https://github.com/nmenon/kernel-test-logs Below is my boot log: ata1: SATA max UDMA/133 mmio [mem 0x4a14-0x4a1410ff] port 0x100 irq 332 mtdoops: mtd device (mtddev=name/number) must be supplied davinci_mdio 48485000.mdio: davinci mdio revision 1.6 davinci_mdio 48485000.mdio: detected phy mask fff9 libphy: 48485000.mdio: probed davinci_mdio 48485000.mdio: phy[1]: device 48485000.mdio:01, driver unknown davinci_mdio 48485000.mdio: phy[2]: device 48485000.mdio:02, driver unknown cpsw 48484000.ethernet: Detected MACID = 74:da:ea:47:7d:9c cpsw 48484000.ethernet: cpsw: Random MACID = ca:39:e7:f3:28:69 Unable to handle kernel paging request at virtual address fffc pgd = c0004000 [fffc] *pgd=affae861, *pte=, *ppte= Internal error: Oops: 37 [#1] SMP ARM Modules linked in: CPU: 1 PID: 1 Comm: swapper/0 Tainted: GW 4.6.0-rc4-next-20160419-2-g94c3d2a #4 Hardware name: Generic DRA74X (Flattened Device Tree) task: ee0c4d80 ti: ee0c6000 task.ti: ee0c6000 PC is at kset_find_obj+0x28/0xa4 LR is at kset_find_obj+0x3c/0xa4 pc : []lr : []psr: a013 sp : ee0c7ec8 ip : fp : c0b005a0 [] (kset_find_obj) from [] (driver_find+0x14/0x30) ata1: SATA link down (SStatus 0 SControl 300) [] (driver_find) from [] (driver_register+0x68/0xf8) [] (driver_register) from [] (do_one_initcall+0x3c/0x178) [] (do_one_initcall) from [] (kernel_init_freeable+0x218/0x2e8) [] (kernel_init_freeable) from [] (kernel_init+0x8/0x118) [] (kernel_init) from [] (ret_from_fork+0x14/0x24) Code: e5954000 e1550004 e2444004 0a0a (e5943000) ---[ end trace 593c492662ed437e ]--- Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b CPU0: stopping CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D W 4.6.0-rc4-next-20160419-2-g94c3d2a #4 Hardware name: Generic DRA74X (Flattened Device Tree) [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0xb0/0xe4) [] (dump_stack) from [] (handle_IPI+0x2c4/0x360) [] (handle_IPI) from [] (gic_handle_irq+0x84/0x94) [] (gic_handle_irq) from [] (__irq_svc+0x58/0x78) Exception stack(0xc0c01f58 to 0xc0c01fa0) 1f40: c0108318 1f60: c0c02a34 c0cbe5f8 c0c029cc c0cbe5f8 1f80: c0b71a38 c0cbd3a9 c0c01fa8 c0108318 c010831c 6013 [] (__irq_svc) from [] (arch_cpu_idle+0x20/0x3c) [] (arch_cpu_idle) from [] (cpu_startup_entry+0x30c/0x3f0) [] (cpu_startup_entry) from [] (start_kernel+0x33c/0x3b4) [] (start_kernel) from [<8000807c>] (0x8000807c) -- regards, -grygorii
