Received-SPF: pass (google.com: domain of linux-kernel+bounces-28711-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>
To: Jiri Pirko <jiri@resnulli.us>
CC: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"vadim.fedorenko@linux.dev" <vadim.fedorenko@linux.dev>,
	"davem@davemloft.net" <davem@davemloft.net>, "Olech, Milena"
	<milena.olech@intel.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "pabeni@redhat.com" <pabeni@redhat.com>,
	"kuba@kernel.org" <kuba@kernel.org>, mschmidt <mschmidt@redhat.com>, "Glaza,
 Jan" <jan.glaza@intel.com>
Subject: RE: [PATCH net v3 2/3] dpll: fix register pin with unregistered
 parent pin
Thread-Topic: [PATCH net v3 2/3] dpll: fix register pin with unregistered
 parent pin
Thread-Index: AQHaR5ClwHLdlm5ZHk+0kNxRBDSVwLDcD+OAgAGtUtA=
Date: Wed, 17 Jan 2024 09:19:35 +0000
Message-ID: <DM6PR11MB4657AD4F70BA302F8DEBEC779B722@DM6PR11MB4657.namprd11.prod.outlook.com>
References: <20240115085241.312144-1-arkadiusz.kubalewski@intel.com>
 <20240115085241.312144-3-arkadiusz.kubalewski@intel.com>
 <ZaYzUXmnS3czrUsG@nanopsycho>
In-Reply-To: <ZaYzUXmnS3czrUsG@nanopsycho>
Accept-Language: pl-PL, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SGvhVMi8O7DdbPJRPZdUAiVGkCXRJ/OIlZfw0xrkLsUG8qphRduBtLniu+Tz?=
 =?us-ascii?Q?+seddm4G+l39HOqHnbaTXtvuNI7cft8eIDQJ6ascGNxVeJp+2VZlxRqt48gu?=
 =?us-ascii?Q?XyV8Trti0tVRcnyY87shGfzRUyi3u/ZOULrQnf2TyzdYXxdr8B3EIGpeFov+?=
 =?us-ascii?Q?9gA0Ze5noguxyYwJRazFHeSkITXs1BmJOcAycRCjVNEPfRfE2DY0fAKSsioA?=
 =?us-ascii?Q?Qkix0Wca7c9S77600+CW0GE6uhy5xxCe5yrpWfGdAtwSXcdlHDMxIiKfAuLF?=
 =?us-ascii?Q?UqpZxkiuKnBsyemu3GIJBo5atg4Ryl4YNA+KGWRD3PUs+D4lR55Pkixv1B1O?=
 =?us-ascii?Q?CSSwH75HQCSyXdPXy+XNjj3xjvqcoUtmUnc0MaG/N2c5ntIaWWXY/6Brbz5t?=
 =?us-ascii?Q?8c0xunNDp/VtO112lEKbn7PzqYrz1kpL/wnmqThw/9lvzkWTZX1Ovcjqb12z?=
 =?us-ascii?Q?oPDah1/6nWsuLzjkEMlHi5WRZK4H6t5c4U8tgCAtxNvMeXvJaRPDNv9EVaUI?=
 =?us-ascii?Q?Z0vL1WoQImJo9tIIyG0sTAQHiyJ28xng2j4waGwlT+2WpeAouVysFXy2087v?=
 =?us-ascii?Q?FbNkgUSWDOJg4VKrWOOs4WB13+Ai3qHv+IyJ3VAwlrbt0Cn4jsPTKK8lT+7U?=
 =?us-ascii?Q?apghe9pDufMtm2ISd1Neb3RbDv2OyT3YwlRs/ygUnidG50Zf0F3PcLFx+Jgn?=
 =?us-ascii?Q?FURFsUJ0jmB8jMhynOaq+YgBgs2RwlqbUf5SvgD4sjcXq2N9OaXe+Q1fPTbn?=
 =?us-ascii?Q?Z4H77T73XypnQ1qJYnHwb/QNWfOZv+kiqYCPEU9Hyz3ZYRJdT+W+XzqNp8ss?=
 =?us-ascii?Q?D8KVOTUk3o/3ihrYZkbN4eP8qeqF3f1d9MaFwjSPQ6wMTWHg9ViTyp/YeWxv?=
 =?us-ascii?Q?0mNaJkWIfrYy1qREoPomYOqjzJy9gekJIfHM5N1pqF6AcVi7rbt72EPCANxZ?=
 =?us-ascii?Q?RiiR0t5QDMsXLAAXWfKgPlL5AXqMnN3xyaNHRG2/E+e6tRszmF8bB35ey3Ss?=
 =?us-ascii?Q?hPjGU6mF9KQFulg9mNOBP5EheWoI+QRgGdFpatAknoYm5C6BljTRicZc1Nj5?=
 =?us-ascii?Q?GzDlMRGG1lRBYOc5A4VaGMT5BFOK7Zp79n4uMdc/qLgb4G+xxDNf4iZfz0lx?=
 =?us-ascii?Q?P1LtzQlnXqEIIYAUZlxC/Vcg/ut5scF4gW03Xvoj5Jy7LNMQv666a6HUGk+L?=
 =?us-ascii?Q?fyulawkeNWRQnIhKifhU96puQVFprzLdLIamRLo11it0A1HpmF9mIBTLNwhJ?=
 =?us-ascii?Q?yGKTk21j27BGvNqBcTbBoGYCYwgmXMhdzMKObIKNyztBlqEhOjKb//ysKdHm?=
 =?us-ascii?Q?3e6XBDEo7uRE6cNQtpj4E6MXv5In3ia6FJNCV/YhQNCt/+0g/qxiXWbQQLVq?=
 =?us-ascii?Q?lpS2bEMRfOGf9gSVLIGuDqEBTni0ItlJB71luFTk0xQpcK2KHXCpVMVdnHRl?=
 =?us-ascii?Q?2LIUv8vPjY9FF96lZxyOiHs1D+H3TJtvk0Kr7ZpRRXru8Bm5Ian+JWX+Gqc/?=
 =?us-ascii?Q?oBKSq4EF+F8viEwOrOy5NHinVhFR2GQghNaozNhSbiSg9FbV3I+V8albqv+V?=
 =?us-ascii?Q?Bo9TnSByEJ4Yje7HG6t5KE+DRmhuJwpu4HtWR75rTwiJxIJ7M+gmrV0I2OBv?=
 =?us-ascii?Q?rw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4657.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9cff50d0-dac2-470c-908a-08dc173d6c2c
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jan 2024 09:19:35.4913
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ZPFOpXD6gXqRLu+1HmxUuQxM74thXKWPKRa13B6dJe/s7cg2BAmz7ffh+bVTwQsaqK8zHKCW2djxuD5sM+3bpsmZqlCnhZ5Wp0HU1x1uzM4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7894
X-OriginatorOrg: intel.com

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Tuesday, January 16, 2024 8:42 AM
>
>Mon, Jan 15, 2024 at 09:52:40AM CET, arkadiusz.kubalewski@intel.com wrote:
>>In case of multiple kernel module instances using the same dpll device:
>>if only one registers dpll device, then only that one can register
>>directly connected pins with a dpll device. When unregistered parent is
>>responsible for determining if the muxed pin can be registered with it
>>or not, the drivers need to be loaded in serialized order to work
>>correctly - first the driver instance which registers the direct pins
>>needs to be loaded, then the other instances could register muxed type
>>pins.
>>
>>Allow registration of a pin with a parent even if the parent was not
>>yet registered, thus allow ability for unserialized driver instance
>>load order.
>>Do not WARN_ON notification for unregistered pin, which can be invoked
>>for described case, instead just return error.
>>
>>Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>>Fixes: 9d71b54b65b1 ("dpll: netlink: Add DPLL framework base functions")
>>Reviewed-by: Jan Glaza <jan.glaza@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>>v3:
>>- allow register with non registered parent dpll device for consistency
>>
>> drivers/dpll/dpll_core.c    | 6 ------
>> drivers/dpll/dpll_netlink.c | 2 +-
>> 2 files changed, 1 insertion(+), 7 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>index fbac32af78b7..69005d8489d3 100644
>>--- a/drivers/dpll/dpll_core.c
>>+++ b/drivers/dpll/dpll_core.c
>>@@ -28,8 +28,6 @@ static u32 dpll_xa_id;
>> 	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>> #define ASSERT_DPLL_NOT_REGISTERED(d)	\
>> 	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>-#define ASSERT_PIN_REGISTERED(p)	\
>>-	WARN_ON_ONCE(!xa_get_mark(&dpll_pin_xa, (p)->id, DPLL_REGISTERED))
>
>Also, for consistency, this could be called from
>__dpll_pin_unregister(). But that is net-next material.
>
>
>Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>

Thank you!
Arkadiusz

>
>
>>
>> struct dpll_device_registration {
>> 	struct list_head list;
>>@@ -613,8 +611,6 @@ dpll_pin_register(struct dpll_device *dpll, struct
>dpll_pin *pin,
>> 	    WARN_ON(!ops->state_on_dpll_get) ||
>> 	    WARN_ON(!ops->direction_get))
>> 		return -EINVAL;
>>-	if (ASSERT_DPLL_REGISTERED(dpll))
>>-		return -EINVAL;
>>
>> 	mutex_lock(&dpll_lock);
>> 	if (WARN_ON(!(dpll->module =3D=3D pin->module &&
>>@@ -692,8 +688,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent,
>>struct dpll_pin *pin,
>> 	    WARN_ON(!ops->state_on_pin_get) ||
>> 	    WARN_ON(!ops->direction_get))
>> 		return -EINVAL;
>>-	if (ASSERT_PIN_REGISTERED(parent))
>>-		return -EINVAL;
>>
>> 	mutex_lock(&dpll_lock);
>> 	ret =3D dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv);
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index 4c64611d32ac..108c002537e6 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -551,7 +551,7 @@ dpll_pin_event_send(enum dpll_cmd event, struct
>>dpll_pin *pin)
>> 	int ret =3D -ENOMEM;
>> 	void *hdr;
>>
>>-	if (WARN_ON(!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED)))
>>+	if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
>> 		return -ENODEV;
>>
>> 	msg =3D genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>--
>>2.38.1
>>