Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Hariprasad Kelam <hkelam@marvell.com>
To:     Jakub Kicinski <kuba@kernel.org>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "willemdebruijn.kernel@gmail.com" <willemdebruijn.kernel@gmail.com>,
        "andrew@lunn.ch" <andrew@lunn.ch>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>,
        Linu Cherian <lcherian@marvell.com>,
        "Geethasowjanya Akula" <gakula@marvell.com>,
        Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
Subject: Re: [Patch v3 net-next 7/7] octeontx2-pf: ethtool physical link
 configuration
Thread-Topic: [Patch v3 net-next 7/7] octeontx2-pf: ethtool physical link
 configuration
Thread-Index: Adb7GrFbqvMxRsGhRYGxDzuFoB4Y/w==
Date:   Thu, 4 Feb 2021 17:37:41 +0000
Message-ID: <MWHPR18MB142117DBE8659D68794E18D3DEB39@MWHPR18MB1421.namprd18.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 63c4afe3-3c91-4b4a-7a48-08d8c93392f6
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Feb 2021 17:37:41.7447
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: xs91Tpms5eqF6bdMSJ8v058NgAtcKaUGKr1j10FBB3qi5zU38IRf966sZDXzolQarv8+ap8SD73c9d95MsVO1w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR18MB3938
X-OriginatorOrg: marvell.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.737
 definitions=2021-02-04_09:2021-02-04,2021-02-04 signatures=0
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jakub,


> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, February 3, 2021 6:59 AM
> To: Hariprasad Kelam <hkelam@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> davem@davemloft.net; willemdebruijn.kernel@gmail.com;
> andrew@lunn.ch; Sunil Kovvuri Goutham <sgoutham@marvell.com>; Linu
> Cherian <lcherian@marvell.com>; Geethasowjanya Akula
> <gakula@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
> Subject: [EXT] Re: [Patch v3 net-next 7/7] octeontx2-pf: ethtool physical=
 link
> configuration
>=20
> On Sun, 31 Jan 2021 18:41:05 +0530 Hariprasad Kelam wrote:
> > From: Christina Jacob <cjacob@marvell.com>
> >
> > Register set_link_ksetting callback with driver such that link
> > configurations parameters like advertised mode,speed, duplex and
> > autoneg can be configured.
> >
> > below command
> > ethtool -s eth0 advertise 0x1 speed 10 duplex full autoneg on
> >
> > Signed-off-by: Christina Jacob <cjacob@marvell.com>
> > Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
> > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> > ---
> >  .../ethernet/marvell/octeontx2/nic/otx2_ethtool.c  | 67
> > ++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> > index d637815..74a62de 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> > @@ -1170,6 +1170,72 @@ static int otx2_get_link_ksettings(struct
> net_device *netdev,
> >  	return 0;
> >  }
> >
> > +static void otx2_get_advertised_mode(const struct ethtool_link_ksettin=
gs
> *cmd,
> > +				     u64 *mode)
> > +{
> > +	u32 bit_pos;
> > +
> > +	/* Firmware does not support requesting multiple advertised modes
> > +	 * return first set bit
> > +	 */
> > +	bit_pos =3D find_first_bit(cmd->link_modes.advertising,
> > +				 __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +	if (bit_pos !=3D __ETHTOOL_LINK_MODE_MASK_NBITS)
> > +		*mode =3D bit_pos;
> > +}
> > +
> > +static int otx2_set_link_ksettings(struct net_device *netdev,
> > +				   const struct ethtool_link_ksettings *cmd) {
> > +	struct otx2_nic *pf =3D netdev_priv(netdev);
> > +	struct ethtool_link_ksettings req_ks;
> > +	struct ethtool_link_ksettings cur_ks;
> > +	struct cgx_set_link_mode_req *req;
> > +	struct mbox *mbox =3D &pf->mbox;
> > +	int err =3D 0;
> > +
> > +	/* save requested link settings */
> > +	memcpy(&req_ks, cmd, sizeof(struct ethtool_link_ksettings));
>=20
> Why do you make this copy? The comment above does not help at all.
>=20
Agreed copy is not necessary . Added this copy for comparing advertised mod=
es with supported modes.
Will fix this in next version.

> > +	memset(&cur_ks, 0, sizeof(struct ethtool_link_ksettings));
> > +
> > +	if (!ethtool_validate_speed(cmd->base.speed) ||
> > +	    !ethtool_validate_duplex(cmd->base.duplex))
> > +		return -EINVAL;
> > +
> > +	if (cmd->base.autoneg !=3D AUTONEG_ENABLE &&
> > +	    cmd->base.autoneg !=3D AUTONEG_DISABLE)
> > +		return -EINVAL;
> > +
> > +	otx2_get_link_ksettings(netdev, &cur_ks);
> > +
> > +	/* Check requested modes against supported modes by hardware */
> > +	if (!bitmap_subset(req_ks.link_modes.advertising,
> > +			   cur_ks.link_modes.supported,
> > +			   __ETHTOOL_LINK_MODE_MASK_NBITS))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&mbox->lock);
> > +	req =3D otx2_mbox_alloc_msg_cgx_set_link_mode(&pf->mbox);
> > +	if (!req) {
> > +		err =3D -ENOMEM;
> > +		goto end;
> > +	}
> > +
> > +	req->args.speed =3D req_ks.base.speed;
> > +	/* firmware expects 1 for half duplex and 0 for full duplex
> > +	 * hence inverting
> > +	 */
> > +	req->args.duplex =3D req_ks.base.duplex ^ 0x1;
> > +	req->args.an =3D req_ks.base.autoneg;
> > +	otx2_get_advertised_mode(&req_ks, &req->args.mode);
>=20
> But that only returns the first bit set. What does the device actually do=
? What
> if the user cleared a middle bit?
>=20
This is initial patch series to support advertised modes. Current firmware =
design is such that
It can handle only one advertised mode. Due to this limitation we are alway=
s checking
The first set bit in advertised modes and passing it to firmware.
Will add multi advertised mode support in near future.

Thanks,
Hariprasad k


> > +	err =3D otx2_sync_mbox_msg(&pf->mbox);
> > +end:
> > +	mutex_unlock(&mbox->lock);
> > +	return err;
> > +}
> > +
> >  static const struct ethtool_ops otx2_ethtool_ops =3D {
> >  	.supported_coalesce_params =3D ETHTOOL_COALESCE_USECS |
> >  				     ETHTOOL_COALESCE_MAX_FRAMES, @@
> -1200,6 +1266,7 @@ static
> > const struct ethtool_ops otx2_ethtool_ops =3D {
> >  	.get_fecparam		=3D otx2_get_fecparam,
> >  	.set_fecparam		=3D otx2_set_fecparam,
> >  	.get_link_ksettings     =3D otx2_get_link_ksettings,
> > +	.set_link_ksettings     =3D otx2_set_link_ksettings,
> >  };
> >
> >  void otx2_set_ethtool_ops(struct net_device *netdev)