linux-kernel - Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f54ad0e2-10d7-40ff-872f-d7c92ae8519b@quicinc.com>
Date: Sat, 7 Dec 2024 00:20:25 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni
	<pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, Andrew Lunn
	<andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_kkumarcs@...cinc.com>, <quic_suruchia@...cinc.com>,
        <quic_pavir@...cinc.com>, <quic_linchen@...cinc.com>,
        <quic_luoj@...cinc.com>, <srinivas.kandagatla@...aro.org>,
        <bartosz.golaszewski@...aro.org>, <vsmuthu@....qualcomm.com>,
        <john@...ozen.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS
 instantiation and phylink operations



On 12/4/2024 11:28 PM, Russell King (Oracle) wrote:
> On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:
>> +static int ipq_pcs_enable(struct phylink_pcs *pcs)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(qpcs_mii->rx_clk);
>> +	if (ret) {
>> +		dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(qpcs_mii->tx_clk);
>> +	if (ret) {
>> +		dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
>> +		clk_disable_unprepare(qpcs_mii->rx_clk);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ipq_pcs_disable(struct phylink_pcs *pcs)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +
>> +	if (__clk_is_enabled(qpcs_mii->rx_clk))
>> +		clk_disable_unprepare(qpcs_mii->rx_clk);
>> +
>> +	if (__clk_is_enabled(qpcs_mii->tx_clk))
>> +		clk_disable_unprepare(qpcs_mii->tx_clk);
> 
> Why do you need the __clk_is_enabled() calls here? Phylink should be
> calling pcs_enable() once when the PCS when starting to use the PCS,
> and then pcs_disable() when it stops using it - it won't call
> pcs_disable() without a preceeding call to pcs_enable().
> 
> Are you seeing something different?
> 

Yes, understand that phylink won't call pcs_disable() without a 
preceeding call to pcs_enable(). However, the "clk_prepare_enable" may 
fail in the pcs_enable() method, so I added the __clk_is_enabled() check 
in pcs_disable() method. This is because the phylink_major_config() 
function today does not interpret the return value of phylink_pcs_enable().

>> +static void ipq_pcs_get_state(struct phylink_pcs *pcs,
>> +			      struct phylink_link_state *state)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +
>> +	switch (state->interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		ipq_pcs_get_state_sgmii(qpcs, index, state);
>> +		break;
>> +	default:
>> +		break;
> ...
>> +static int ipq_pcs_config(struct phylink_pcs *pcs,
>> +			  unsigned int neg_mode,
>> +			  phy_interface_t interface,
>> +			  const unsigned long *advertising,
>> +			  bool permit)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +
>> +	switch (interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
>> +	default:
>> +		dev_err(qpcs->dev,
>> +			"Unsupported interface %s\n", phy_modes(interface));
>> +		return -EOPNOTSUPP;
>> +	};
>> +}
>> +
>> +static void ipq_pcs_link_up(struct phylink_pcs *pcs,
>> +			    unsigned int neg_mode,
>> +			    phy_interface_t interface,
>> +			    int speed, int duplex)
>> +{
>> +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>> +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>> +	int index = qpcs_mii->index;
>> +	int ret;
>> +
>> +	switch (interface) {
>> +	case PHY_INTERFACE_MODE_SGMII:
>> +	case PHY_INTERFACE_MODE_QSGMII:
>> +		ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
>> +						   neg_mode, speed);
>> +		break;
>> +	default:
>> +		dev_err(qpcs->dev,
>> +			"Unsupported interface %s\n", phy_modes(interface));
>> +		return;
>> +	}
> 
> So you only support SGMII and QSGMII. Rather than checking this in every
> method implementation, instead provide a .pcs_validate method that
> returns an error for unsupported interfaces please.
> 

Yes, I can add the pcs_validate() method to validate the link 
configurations. This will catch invalid interface mode during the PCS 
initialization time, earlier than the pcs_config and pcs_link_up contexts.

But after of the PCS init, if at a later point the PHY interface mode 
changes, it seems phylink today is not calling the pcs_validate() op to 
validate the changed new interface mode at the time of 
"phylink_resolve". (Hope my understanding is correct).
So, In the pcs ops methods, I will keep the switch case to check and 
handle the unsupported interface modes.

> Thanks.
>