Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Wed, 22 Nov 2023 17:48:28 +0100
From:   =?UTF-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Russ Weight <russ.weight@linux.dev>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller
 driver
Message-ID: <20231122174828.7625d7f4@kmaincent-XPS-13-7390>
In-Reply-To: <45694d77-bcf8-4377-9aa0-046796de8d74@lunn.ch>
References: <20231116-feature_poe-v1-0-be48044bf249@bootlin.com>
        <20231116-feature_poe-v1-9-be48044bf249@bootlin.com>
        <45694d77-bcf8-4377-9aa0-046796de8d74@lunn.ch>
Organization: bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, 18 Nov 2023 19:54:30 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

>=20
> > +	unsigned long last_cmd_key_time;
> > +
> > +	enum ethtool_pse_admin_state
> > admin_state[PD692X0_MAX_LOGICAL_PORTS]; +};
> > +
> > +/* Template list of the fixed bytes of the communication messages */
> > +static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_=
CNT]
> > =3D {
> > +	[PD692X0_MSG_RESET] =3D {
> > +		.content =3D {
> > +			.key =3D PD692X0_KEY_CMD,
> > +			.sub =3D {0x07, 0x55, 0x00},
> > +			.data =3D {0x55, 0x00, 0x55, 0x4e,
> > +				 0x4e, 0x4e, 0x4e, 0x4e},
> > +		},
> > +	}, =20
>=20
> Is there any documentation about what all these magic number mean?
>=20
> > +/* Implementation of the i2c communication in particular when there is
> > + * a communication loss. See the "Synchronization During Communication
> > Loss"
> > + * paragraph of the Communication Protocol document.
> > + */ =20
>=20
> Is this document public?

Yes:
https://www.microchip.com/en-us/software-library/p3-55-firmware-for-pd69200=
-for-ieee802-3bt

>=20
> > +static int pd692x0_recv_msg(struct pd692x0_priv *priv,
> > +			    struct pd692x0_msg *msg,
> > +			    struct pd692x0_msg_content *buf)
> > +{
> > +	const struct i2c_client *client =3D priv->client;
> > +	int ret;
> > +
> > +	memset(buf, 0, sizeof(*buf));
> > +	if (msg->delay_recv)
> > +		msleep(msg->delay_recv);
> > +	else
> > +		msleep(30);
> > +
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		goto out; =20
>=20
> This is the first attempt to receive the message. I assume buf->key
> not being 0 indicates something has been received?

Yes,=20

>=20
> > +
> > +	msleep(100);
> > +
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		goto out; =20
>=20
> So this is a second attempt. Should there be another memset? Could the
> first failed transfer fill the buffer with random junk in the higher
> bytes, and a successful read here could be a partial read and the end
> of the buffer still contains the junk.

Not sure. The message read should have the right size.
I will maybe add the memset to prevent any weird behavior.

> Another resend and two more attempts to receive.
>=20
> Is there a reason to not uses for loops here? And maybe put
> send/receive/receive into a helper? And maybe make the first send part
> of this, rather then separate? I think the code will be more readable
> when restructured.

I have written it like that to describe literally the communication loss
procedure. I may restructured it for better understanding.

> > +static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
> > +				      unsigned long id,
> > +				      struct netlink_ext_ack *extack,
> > +				      const struct pse_control_config
> > *config) +{
> > +	struct pd692x0_priv *priv =3D to_pd692x0_priv(pcdev);
> > +	struct pd692x0_msg_content buf =3D {0};
> > +	struct pd692x0_msg msg;
> > +	int ret;
> > +
> > +	ret =3D pd692x0_fw_unavailable(priv);
> > +	if (ret)
> > +		return ret; =20
>=20
> It seems a bit late to check if the device has any firmware. I would
> of expected probe to check that, and maybe attempt to download
> firmware. If that fails, fail the probe, since the PSE is a brick.

The PSE can still be flashed it never become an unusable brick.
We can flash the wrong Firmware, or having issue in the flashing process. T=
his
way we could reflash the controller firmware several times.=20

> > +static struct pd692x0_msg_ver pd692x0_get_sw_version(struct pd692x0_pr=
iv
> > *priv) +{
> > +	struct pd692x0_msg msg =3D
> > pd692x0_msg_template_list[PD692X0_MSG_GET_SW_VER];
> > +	struct device *dev =3D &priv->client->dev;
> > +	struct pd692x0_msg_content buf =3D {0};
> > +	struct pd692x0_msg_ver ver =3D {0};
> > +	int ret;
> > +
> > +	ret =3D pd692x0_sendrecv_msg(priv, &msg, &buf);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to get PSE version (%pe)\n",
> > ERR_PTR(ret));
> > +		return ver; =20
>=20
> I _think_ that return is wrong ???

No, this return will return an empty struct pd692x0_msg_ver.
Which means the firmware has not any version.

> Is the firmware in Motorola SREC format? I thought the kernel had a
> helper for that, but a quick search did not find it. So maybe i'm
> remembering wrongly. But it seems silly for every driver to implement
> an SREC parser.

Oh, I didn't know this format. The firmware seems indeed to match this form=
at
specification.
I found two reference of this Firmware format in the kernel:
https://elixir.bootlin.com/linux/v6.5.7/source/sound/soc/codecs/zl38060.c#L=
178
https://elixir.bootlin.com/linux/v6.5.7/source/drivers/staging/wlan-ng/pris=
m2fw.c

Any preference in the choice? The zl38060 fw usage is maybe the easiest.

Regards,
--=20
K=C3=B6ry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com