[PATCH] net: fec: add support of ethtool get_regs

[PATCH] net: fec: add support of ethtool get_regs

[Philippe Reynes]

This enables the ethtool's "-d" and "--register-dump"
options for fec devices.

Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 66d47e4..07aee1f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2118,6 +2118,28 @@ static void fec_enet_get_drvinfo(struct net_device *ndev,
 	strlcpy(info->bus_info, dev_name(&ndev->dev), sizeof(info->bus_info));
 }
 
+static int fec_enet_get_regs_len(struct net_device *ndev)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct resource *r;
+	int s = 0;
+
+	r = platform_get_resource(fep->pdev, IORESOURCE_MEM, 0);
+	if (r) {
+		s = resource_size(r);
+	}
+
+	return s;
+}
+
+static void fec_enet_get_regs(struct net_device *ndev,
+			      struct ethtool_regs *regs, void *regbuf)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	memcpy_fromio(regbuf, fep->hwp, regs->len);
+}
+
 static int fec_enet_get_ts_info(struct net_device *ndev,
 				struct ethtool_ts_info *info)
 {
@@ -2515,6 +2537,8 @@ static const struct ethtool_ops fec_enet_ethtool_ops = {
 	.get_settings		= fec_enet_get_settings,
 	.set_settings		= fec_enet_set_settings,
 	.get_drvinfo		= fec_enet_get_drvinfo,
+	.get_regs_len		= fec_enet_get_regs_len,
+	.get_regs		= fec_enet_get_regs,
 	.nway_reset		= fec_enet_nway_reset,
 	.get_link		= ethtool_op_get_link,
 	.get_coalesce		= fec_enet_get_coalesce,
-- 
1.7.4.4

Re: [PATCH] net: fec: add support of ethtool get_regs

[Russell King - ARM Linux]

On Sat, May 09, 2015 at 10:52:08PM +0200, Philippe Reynes wrote:
> +static void fec_enet_get_regs(struct net_device *ndev,
> +			      struct ethtool_regs *regs, void *regbuf)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +
> +	memcpy_fromio(regbuf, fep->hwp, regs->len);

Using memcpy_fromio() to copy device registers is not a good idea -
it can use a variable access size which can cause bus faults.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] net: fec: add support of ethtool get_regs

[David Miller]

From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Sat, 9 May 2015 22:17:46 +0100

> On Sat, May 09, 2015 at 10:52:08PM +0200, Philippe Reynes wrote:
>> +static void fec_enet_get_regs(struct net_device *ndev,
>> +			      struct ethtool_regs *regs, void *regbuf)
>> +{
>> +	struct fec_enet_private *fep = netdev_priv(ndev);
>> +
>> +	memcpy_fromio(regbuf, fep->hwp, regs->len);
> 
> Using memcpy_fromio() to copy device registers is not a good idea -
> it can use a variable access size which can cause bus faults.

Agreed.

Re: [PATCH] net: fec: add support of ethtool get_regs

[Fabio Estevam]

Philippe,

On Sat, May 9, 2015 at 6:17 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> Using memcpy_fromio() to copy device registers is not a good idea -
> it can use a variable access size which can cause bus faults.

An example on how memcpy_fromio() can be avoided in get_regs:
drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c

Regards,

Fabio Estevam

Re: [PATCH] net: fec: add support of ethtool get_regs

[Philippe Reynes]

Hi Fabio,

On 09/05/15 23:59, Fabio Estevam wrote:
> Philippe,
>
> On Sat, May 9, 2015 at 6:17 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk>  wrote:
>
>> Using memcpy_fromio() to copy device registers is not a good idea -
>> it can use a variable access size which can cause bus faults.
>
> An example on how memcpy_fromio() can be avoided in get_regs:
> drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c

Thanks for pointing me this example. I've already send a patch,
and I've used drivers/net/ethernet/freescale/gianfar_ethtool.c
as example. I hope it's a good example too.

> Regards,
>
> Fabio Estevam

Regards,
Philippe

Re: [PATCH] net: fec: add support of ethtool get_regs

[David Miller]

From: Philippe Reynes <tremyfr@gmail.com>
Date: Sun, 10 May 2015 00:16:21 +0200

> Hi Fabio,
> 
> On 09/05/15 23:59, Fabio Estevam wrote:
>> Philippe,
>>
>> On Sat, May 9, 2015 at 6:17 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk>  wrote:
>>
>>> Using memcpy_fromio() to copy device registers is not a good idea -
>>> it can use a variable access size which can cause bus faults.
>>
>> An example on how memcpy_fromio() can be avoided in get_regs:
>> drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
> 
> Thanks for pointing me this example. I've already send a patch,
> and I've used drivers/net/ethernet/freescale/gianfar_ethtool.c
> as example. I hope it's a good example too.

I think you need to be much more careful and conservative in your
implementation.

You should skip I/O addresses that don't have defined registers
at those offsets for the chip in question.

Also, you should _very_ carefully evaluate each and every register you
dump and potentially skip certain registers which have strong negative
side effects if read arbitrarily.

For example, dumping the interrupt status register could cause pending
interrupt status to be cleared, and thus cause the driver to lose
interrupts and subsequently packet processing will hang.

Re: [PATCH] net: fec: add support of ethtool get_regs

[Philippe Reynes]

Hi david,

On 10/05/15 03:01, David Miller wrote:
> From: Philippe Reynes<tremyfr@gmail.com>
> Date: Sun, 10 May 2015 00:16:21 +0200
>
>> Hi Fabio,
>>
>> On 09/05/15 23:59, Fabio Estevam wrote:
>>> Philippe,
>>>
>>> On Sat, May 9, 2015 at 6:17 PM, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk>   wrote:
>>>
>>>> Using memcpy_fromio() to copy device registers is not a good idea -
>>>> it can use a variable access size which can cause bus faults.
>>>
>>> An example on how memcpy_fromio() can be avoided in get_regs:
>>> drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
>>
>> Thanks for pointing me this example. I've already send a patch,
>> and I've used drivers/net/ethernet/freescale/gianfar_ethtool.c
>> as example. I hope it's a good example too.
>
> I think you need to be much more careful and conservative in your
> implementation.
>
> You should skip I/O addresses that don't have defined registers
> at those offsets for the chip in question.

Ok, I've added an array with all register, so I only read defined registers.

> Also, you should _very_ carefully evaluate each and every register you
> dump and potentially skip certain registers which have strong negative
> side effects if read arbitrarily.
>
> For example, dumping the interrupt status register could cause pending
> interrupt status to be cleared, and thus cause the driver to lose
> interrupts and subsequently packet processing will hang.

Thanks for the feedback. I've read all the register, and all registers
can be read without negative side effect. Even interrupt status register,
interrupt are cleared when the register EIR is written (not read).

Regards,
Philippe