From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 364B419D07E; Thu, 5 Jun 2025 05:00:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=205.220.168.131 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1749099626; cv=none; b=VPZ44c44Hev0km42zfC4NfhURgEp0sUMRUmjhqc5Y3gpFHFeBRrfeh32JCEGzrcVD7GhO+M33NwMdv5cyVSlZR5UhWQGTwXeuB+4CJ7ql2BK//nLT58Y4RqMu3Xftd/0vaRSP4c8/hsmscOdo4bNXjoAKFuBWkeIE1rY5XOvucg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1749099626; c=relaxed/simple; bh=8ei//ic++rZxYT54f37/ii3rswY6C83GCS9YizWM/+k=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=KYsIHU+QzBrnvHB9eatfNcL6vGBhiyxqF57r3dmW1/AIpODDmzj2U8jprcsN2CnZ0OH66o5PQE/gzqIREv0qentuPNOH4lUHhFC0Y1kUcxSFjET8LSEVqOrnx8gR6uz+mzikoNqaWmsov5zlVh0Ae0+recuPc52NivbcujR6P34= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com; spf=pass smtp.mailfrom=quicinc.com; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b=PzhnlvL5; arc=none smtp.client-ip=205.220.168.131 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=quicinc.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b="PzhnlvL5" Received: from pps.filterd (m0279865.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 554GBiTL007516; Thu, 5 Jun 2025 04:59:59 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=qcppdkim1; bh= VCDAPDZBMd3Ybf7L7X6pzUCk1WNQr1j1w6rtdHb7Hvc=; b=PzhnlvL5GHOYQTjC dkKPbrvE6abw/IQbrd8DXs8n1GKPOZmiv2AWjrdV6boreoCJXTeGAEwS6A8gONkR lMhjeUqZG0TbN6IXP0nEchRasId3Wz0VQXhtU2QUg8IPFqBBsqne9NS1JYx+rd2X p0fcytrs+LcSgWfm1GwaNNCxxCtpyr82Wj7l4bCL7FrPCftujd4uVYy/3ddx9gwR uKC6yId0OX6zSJUb07w38/T9YWKt7KJxBwY2ZWG7UbLK5dbms6hCksVEDfOPKcZX /NK3udCes0t7p14tGlQRiLN4gctFAXjq++VBggyJqyyjKOzo9SYHofv0x4ZydtWR CQoYZw== Received: from nasanppmta05.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 471g8t03kp-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 05 Jun 2025 04:59:58 +0000 (GMT) Received: from nasanex01b.na.qualcomm.com (nasanex01b.na.qualcomm.com [10.46.141.250]) by NASANPPMTA05.qualcomm.com (8.18.1.2/8.18.1.2) with ESMTPS id 5554xwqY032285 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 5 Jun 2025 04:59:58 GMT Received: from [10.216.16.95] (10.80.80.8) by nasanex01b.na.qualcomm.com (10.46.141.250) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.9; Wed, 4 Jun 2025 21:59:53 -0700 Message-ID: Date: Thu, 5 Jun 2025 10:29:49 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 net] net: phy: clear phydev->devlink when the link is deleted To: "Abhishek Chauhan (ABC)" , Wei Fang CC: Florian Fainelli , "andrew@lunn.ch" , "hkallweit1@gmail.com" , "linux@armlinux.org.uk" , "davem@davemloft.net" , "edumazet@google.com" , "kuba@kernel.org" , "pabeni@redhat.com" , "xiaolei.wang@windriver.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "imx@lists.linux.dev" References: <20250523083759.3741168-1-wei.fang@nxp.com> <8b947cec-f559-40b4-a0e0-7a506fd89341@gmail.com> <0b44c0f5-d922-4d89-8244-f114aedafa03@quicinc.com> Content-Language: en-US From: Sarosh Hasan In-Reply-To: <0b44c0f5-d922-4d89-8244-f114aedafa03@quicinc.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01b.na.qualcomm.com (10.46.141.250) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Authority-Analysis: v=2.4 cv=RMizH5i+ c=1 sm=1 tr=0 ts=6841244f cx=c_pps a=JYp8KDb2vCoCEuGobkYCKw==:117 a=JYp8KDb2vCoCEuGobkYCKw==:17 a=jZjJzmEmTJcjZ5Ws:21 a=GEpy-HfZoHoA:10 a=IkcTkHD0fZMA:10 a=6IFa9wvqVegA:10 a=8AirrxEcAAAA:8 a=zo4qo1mtkRo1JUt1IrUA:9 a=3ZKOabzyN94A:10 a=QEXdDO2ut3YA:10 a=ST-jHhOKWsTCqRlWije3:22 X-Proofpoint-Spam-Details-Enc: AW1haW4tMjUwNjA1MDA0MSBTYWx0ZWRfX8RWzedTfUh6e 5YG4jULnTUzWwLPHofQqTiSZLxugihItcoOTG/pj336/XqASz0UCmCEACyVG9x5UcZnXX6ODxGT 9916m7x7Sh8SWyROZFlIEOo1JBSi5jkCs2il2TAdq/rJSlzNAhHJfPXi5DTgkP4Y/oHbQJOVLcO B+6q6z4/tDo5HCCKMLB8eLfJq4MxhxLScA/GtxyRJUgudV7AqSsLkfqyRXhc2WKhUPXuACsL9+r qfxfoopYDJkoAoNZc9jehSRJ/kQlhqsFFzCMh5Pm4G2DTmzttUcIufCBCZc4MurltK/cKzaoVjp pfDGf3WgW2/ZZKMiGb2vXGqucL66aRgd4Tc/pIHDGrwdWPZ1uwNWfvrHqS4IZjSScBlzf/5kqnO FdYnCR3u9HWlNFrtRLo2xqXDqO8Hit7rnAmR0Cj2nODFhufY45ocV6V1JoWs4lrtm5TsVaIF X-Proofpoint-GUID: T9sPq_QfEuKn_ROq-2DGTnMeSboqpXFm X-Proofpoint-ORIG-GUID: T9sPq_QfEuKn_ROq-2DGTnMeSboqpXFm X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1099,Hydra:6.0.736,FMLib:17.12.80.40 definitions=2025-06-05_01,2025-06-03_02,2025-03-28_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 phishscore=0 spamscore=0 lowpriorityscore=0 malwarescore=0 bulkscore=0 suspectscore=0 adultscore=0 mlxlogscore=999 mlxscore=0 clxscore=1011 priorityscore=1501 classifier=spam authscore=0 authtc=n/a authcc= route=outbound adjust=0 reason=mlx scancount=1 engine=8.19.0-2505280000 definitions=main-2506050041 On 6/4/2025 11:39 AM, Abhishek Chauhan (ABC) wrote: > > > On 6/3/2025 11:00 PM, Wei Fang wrote: >>>> On 5/23/2025 1:37 AM, Wei Fang wrote: >>>>> There is a potential crash issue when disabling and re-enabling the >>>>> network port. When disabling the network port, phy_detach() calls >>>>> device_link_del() to remove the device link, but it does not clear >>>>> phydev->devlink, so phydev->devlink is not a NULL pointer. Then the >>>>> network port is re-enabled, but if phy_attach_direct() fails before >>>>> calling device_link_add(), the code jumps to the "error" label and >>>>> calls phy_detach(). Since phydev->devlink retains the old value from >>>>> the previous attach/detach cycle, device_link_del() uses the old value, >>>>> which accesses a NULL pointer and causes a crash. The simplified crash >>>>> log is as follows. >>>>> >>>>> [   24.702421] Call trace: >>>>> [   24.704856]  device_link_put_kref+0x20/0x120 >>>>> [   24.709124]  device_link_del+0x30/0x48 >>>>> [   24.712864]  phy_detach+0x24/0x168 >>>>> [   24.716261]  phy_attach_direct+0x168/0x3a4 >>>>> [   24.720352]  phylink_fwnode_phy_connect+0xc8/0x14c >>>>> [   24.725140]  phylink_of_phy_connect+0x1c/0x34 >>>>> >>>>> Therefore, phydev->devlink needs to be cleared when the device link is >>>>> deleted. >>>>> >>>>> Fixes: bc66fa87d4fd ("net: phy: Add link between phy dev and mac dev") >>>>> Signed-off-by: Wei Fang >>>> >>> @Wei >>> What happens in case of shared mdio ? >>> >>> 1. Device 23040000 has the mdio node of both the ethernet phy and device >>> 23000000 references the phy-handle present in the Device 23040000 >>> 2. When rmmod of the driver happens >>> 3. the parent devlink is already deleted. >>> 4. This cause the child mdio to access an entry causing a corruption. >>> 5. Thought this fix would help but i see that its not helping the case. >>> >> >> My patch is only to fix the potential crash issue when re-enabling >> the network interface. phy_detach() is not called when the MDIO >> controller driver is removed. So phydev->devlink is not cleared, but >> actually the device link has been removed by phy_device_remove() >> --> device_del(). Therefore, it will cause the crash when the MAC >> controller driver is removed. >> >>> Wondering if this is a legacy issue with shared mdio framework. >>> >> >> I think this issue is also introduced by the commit bc66fa87d4fd >> ("net: phy: Add link between phy dev and mac dev"). I suggested >> to change the DL_FLAG_STATELESS flag to >> DL_FLAG_AUTOREMOVE_SUPPLIER to solve this issue, so that >> the consumer (MAC controller) driver will be automatically removed >> when the link is removed. The changes are as follows. >> > > thanks a lot , Russell and Wei for your prompt response. > I appreciate your help. let me test this change and get back. > Myself and Abhishek tested this suggestion from Wei and we now observe that the driver removal is successful without a crash. It looks like the flag 'DL_FLAG_AUTOREMOVE_SUPPLIER' is helping. Wei and Russell, please suggest the next steps. >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 73f9cb2e2844..a6d7acd73391 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -1515,6 +1515,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, >> struct mii_bus *bus = phydev->mdio.bus; >> struct device *d = &phydev->mdio.dev; >> struct module *ndev_owner = NULL; >> + struct device_link *devlink; >> bool using_genphy = false; >> int err; >> >> @@ -1646,9 +1647,16 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, >> * another mac interface, so we should create a device link between >> * phy dev and mac dev. >> */ >> - if (dev && phydev->mdio.bus->parent && dev->dev.parent != phydev->mdio.bus->parent) >> - phydev->devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev, >> - DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS); >> + if (dev && phydev->mdio.bus->parent && >> + dev->dev.parent != phydev->mdio.bus->parent) { >> + devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev, >> + DL_FLAG_PM_RUNTIME | >> + DL_FLAG_AUTOREMOVE_SUPPLIER); >> + if (!devlink) { >> + err = -ENOMEM; >> + goto error; >> + } >> + } >> >> return err; >> >> @@ -1749,11 +1757,6 @@ void phy_detach(struct phy_device *phydev) >> struct module *ndev_owner = NULL; >> struct mii_bus *bus; >> >> - if (phydev->devlink) { >> - device_link_del(phydev->devlink); >> - phydev->devlink = NULL; >> - } >> - >> if (phydev->sysfs_links) { >> if (dev) >> sysfs_remove_link(&dev->dev.kobj, "phydev"); >> diff --git a/include/linux/phy.h b/include/linux/phy.h >> index e194dad1623d..cc1f45c3ff21 100644 >> --- a/include/linux/phy.h >> +++ b/include/linux/phy.h >> @@ -505,8 +505,6 @@ struct macsec_ops; >> * >> * @mdio: MDIO bus this PHY is on >> * @drv: Pointer to the driver for this PHY instance >> - * @devlink: Create a link between phy dev and mac dev, if the external phy >> - * used by current mac interface is managed by another mac interface. >> * @phyindex: Unique id across the phy's parent tree of phys to address the PHY >> * from userspace, similar to ifindex. A zero index means the PHY >> * wasn't assigned an id yet. >> @@ -610,8 +608,6 @@ struct phy_device { >> /* And management functions */ >> const struct phy_driver *drv; >> >> - struct device_link *devlink; >> - >> u32 phyindex; >> u32 phy_id; >>