Date:   Thu, 16 Mar 2023 11:05:24 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Markus Schneider-Pargmann <msp@baylibre.com>
Cc:     Marc Kleine-Budde <mkl@pengutronix.de>,
        Chandrasekar Ramakrishnan <rcsekar@samsung.com>,
        Wolfgang Grandegger <wg@grandegger.com>,
        Vincent MAILHOL <mailhol.vincent@wanadoo.fr>,
        linux-can@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/16] can: m_can: Add rx coalescing ethtool support
Message-ID: <ZBLp5G4qARbScJw0@corigine.com>
References: <20230315110546.2518305-1-msp@baylibre.com>
 <20230315110546.2518305-10-msp@baylibre.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230315110546.2518305-10-msp@baylibre.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?TM7Jo+h7oLPM/EbAJn3L+rOuk+nWp/y+pAraO8Ex+VErTMr8Em66Ny/YsvdI?=
 =?us-ascii?Q?3an+h+YD4Pe8A6NMVWxIpCIP4xGVq7kBIZiQiJoDmmTW8SRrClpUyAzJqzmy?=
 =?us-ascii?Q?4Lc2Nr/E09pIbNEfOhc9UbgEtNcyFAFrUZNkZtbDZ9kSbVSW30u0WoP+dFAr?=
 =?us-ascii?Q?54/iYYyOKE2FMWnSo66g2FEONvu5raMOMQl5T7sD5oF6TGPK15VEnnY6IvCW?=
 =?us-ascii?Q?TfpkEw/r80ekc/cvZc77wBoUEHnT98EOBQQGp3q5VAS+j5sud9pfeJa62wr0?=
 =?us-ascii?Q?miuA6NU3o5IAgqw+DBTKkhTIIGFMd3NUx8rF3aCKGxkjJQmT1XmFiTVSBOl8?=
 =?us-ascii?Q?AqPzapNTW4GxdsyxsJkQRPzvDkyTn62yvLvmUGXLeDPAhWBZbaYtmuCAWsDb?=
 =?us-ascii?Q?o8zYvwDZa9CRuJLbsrWqUgBTVyaTBrnHkeFlJATYJsf8WJNUL6bA4BvhOo+M?=
 =?us-ascii?Q?OOWv/WRL7qJzGWGt8uUDndH6YgOZ/p4OGGcsAN4Y5IjILw88NbndZPb/9pPf?=
 =?us-ascii?Q?TiUAHL03j/6cDD15o+9C5Y+K4B0fuuWU2CP24uaXuIXwTYYFlafGePiAToyG?=
 =?us-ascii?Q?3BzzmCH7BOvHkyqeENuf/S/XQOReGlnHwUPiH2ouF2KsIOq861XXe83Ck4us?=
 =?us-ascii?Q?f+wBJOwVhCt71UJuw2F7ySTbx/hugpboa232GEau8jiP/1czCOglbLhrRwJy?=
 =?us-ascii?Q?S5XxuOPao9BNnZ8szT8D7M1MXUtoi5hoQFhsNuA2nln2L4Cxvm/9p2Vsh9TW?=
 =?us-ascii?Q?d1qYn1KWEsinOEy4NVJxkWILMyTI+0aE9OwYSreA20WNdPTTrKB9b1cVsU3P?=
 =?us-ascii?Q?MsB6rSdji5LWyvCHTb/CmhZ3iJV1yJr0kE7u2jfSK0NsRaKU1itpuiI1qw02?=
 =?us-ascii?Q?SIn+AUvI7XpmV1ufh/u9LqihVg2ch974iLnkDiVPAin4L9/LTurCVQ7DENEZ?=
 =?us-ascii?Q?br1k7ClBhNS8uPLzBECswi2xMU1MuUFuwA/d06rJkqDmOCTBDI+xgMcVxdPd?=
 =?us-ascii?Q?N8V3uRJyUcjW0Ngj/lfZrqN6Cyx0/fAxx25Q/H72E/BPV5yNVk8UHED+lf88?=
 =?us-ascii?Q?QmhBv31nihdRDIryhILrAcvcSCq++KuU5sIeKgTLqrXXqAU8o6oUOr9r3vmT?=
 =?us-ascii?Q?XTkjwmeioA1rI8f7KzzT+HOnYzVt139hBRunzg49SKG6xUMxtjFL7j7HFTqP?=
 =?us-ascii?Q?IcBDf3ewB1rpE/n5UkJCW2Gr88xtPLXZ3YYFivFrNtOcOWs3agg9sMvKLGxn?=
 =?us-ascii?Q?c2FcfKCRRLb1DJa9/13Y/WIuYajIvpCSZyytSurfywgiQaJgnmlMd3rcth2B?=
 =?us-ascii?Q?gFcR4d5cyOOhO4Trde/PFO6Et5uKzADtjZV5Tfm3jBRfnmIH8MbokDHLdit2?=
 =?us-ascii?Q?Qf7g2jjZxRn3BXziuDPwaoPy/IJMjuhnEHU57zoPrz5IJiRQa1YCrVkkspPf?=
 =?us-ascii?Q?f0gCSU+/6G4JgCqIuqnGJ/r1NYdvOYC9M5fZBHQ6gXfaZQi1bsvSM1o1qPYh?=
 =?us-ascii?Q?HwX+FV3YBkbFdEBYZ1S39Gx9pPjuWKLODJ9ngH5xnKpn+5RNl+8Tmu4JR2DH?=
 =?us-ascii?Q?xKyQQzIdT7iDmVg0JR7LF/qvXyQSgTEdsLYA2r943nn8HiVaHppoQFNcqCli?=
 =?us-ascii?Q?EEv4KaqmWVbq4lP3WrsO70Ttg0Bm7AYo/FGFxTXPRfGzncD1PDnn4q+CDAgh?=
 =?us-ascii?Q?jI0hew=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 455a3ba5-3461-4ee7-d2e8-08db2605f935
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Mar 2023 10:05:30.2053
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3q4e3jirFICtplEj8Nv22f4g70ArB3NCc4XHOPhQngdWLlF5xwoTh9Jh5VzQqB1BgfSsFkGsY21OVXnwSpc4MCd30mhZOSqwIiqzV15OMF0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR13MB4066
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Mar 15, 2023 at 12:05:39PM +0100, Markus Schneider-Pargmann wrote:
> Add the possibility to set coalescing parameters with ethtool.
> 
> rx-frames-irq and rx-usecs-irq can only be set and unset together as the
> implemented mechanism would not work otherwise. rx-frames-irq can't be
> greater than the RX FIFO size.
> 
> Also all values can only be changed if the chip is not active.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>

Nit below not withstanding,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> ---
>  drivers/net/can/m_can/m_can.c | 46 +++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 94c962ac6992..7f8decfae81e 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c

...

> +static int m_can_set_coalesce(struct net_device *dev,
> +			      struct ethtool_coalesce *ec,
> +			      struct kernel_ethtool_coalesce *kec,
> +			      struct netlink_ext_ack *ext_ack)
> +{
> +	struct m_can_classdev *cdev = netdev_priv(dev);
> +
> +	if (cdev->can.state != CAN_STATE_STOPPED) {
> +		netdev_err(dev, "Device is in use, please shut it down first\n");
> +		return -EBUSY;
> +	}
> +
> +	if (ec->rx_max_coalesced_frames_irq > cdev->mcfg[MRAM_RXF0].num) {
> +		netdev_err(dev, "rx-frames-irq %u greater than the RX FIFO %u\n",
> +			   ec->rx_max_coalesced_frames_irq,
> +			   cdev->mcfg[MRAM_RXF0].num);
> +		return -EINVAL;
> +	}
> +	if ((ec->rx_max_coalesced_frames_irq == 0) != (ec->rx_coalesce_usecs_irq == 0)) {

nit: checkpatch complains about unnecessary parentheses on the line above.

drivers/net/can/m_can/m_can.c:1970: CHECK: Unnecessary parentheses around 'ec->rx_max_coalesced_frames_irq == 0'
+	if ((ec->rx_max_coalesced_frames_irq == 0) != (ec->rx_coalesce_usecs_irq == 0)) {

drivers/net/can/m_can/m_can.c:1970: CHECK: Unnecessary parentheses around 'ec->rx_coalesce_usecs_irq == 0'
+	if ((ec->rx_max_coalesced_frames_irq == 0) != (ec->rx_coalesce_usecs_irq == 0)) {

> +		netdev_err(dev, "rx-frames-irq and rx-usecs-irq can only be set together\n");
> +		return -EINVAL;
> +	}
> +
> +	cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
> +	cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
> +
> +	return 0;
> +}

...