Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Lu, Brent" <brent.lu@intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
CC:     "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "Rojewski, Cezary" <cezary.rojewski@intel.com>,
        Liam Girdwood <liam.r.girdwood@linux.intel.com>,
        Peter Ujfalusi <peter.ujfalusi@linux.intel.com>,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Mark Brown <broonie@kernel.org>,
        "Jaroslav Kysela" <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Zhi, Yong" <yong.zhi@intel.com>,
        Ajye Huang <ajye_huang@compal.corp-partner.google.com>,
        "Bhat, Uday M" <uday.m.bhat@intel.com>,
        Terry Cheong <htcheong@chromium.org>,
        "Chiang, Mac" <mac.chiang@intel.com>,
        "R, Dharageswari" <dharageswari.r@intel.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Subject: RE: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI
Thread-Topic: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from
 ACPI
Thread-Index: AQHZuqoaXCiyShA7oEmUALJNRLZwbK/Ip3CAgAAMRwCAABEQ8A==
Date:   Mon, 24 Jul 2023 11:06:02 +0000
Message-ID: <CY5PR11MB62579D3B679BB437017529D79702A@CY5PR11MB6257.namprd11.prod.outlook.com>
References: <20230720092628.758834-1-brent.lu@intel.com>
 <20230720092628.758834-2-brent.lu@intel.com>
 <dc6de509-6984-1434-b53f-9600e8bc7c49@linux.intel.com>
 <ZL5Jzod5NBETv9Dp@smile.fi.intel.com>
In-Reply-To: <ZL5Jzod5NBETv9Dp@smile.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?yoQXSjFXpIOs0Vk+/igd7KoDCp4tCQr+w2voDRHImDNI8jDCbatAZ8ZyFIsN?=
 =?us-ascii?Q?B3zi7TpqhElo8S8IXD6EMLuRA2nl0uINwjmeooA6ittH4j9euM3NtMYb2VrL?=
 =?us-ascii?Q?8ENHdxFGV3nBBFaTxJK/4dSE0rKvtSibN1Sz+1Q3n261xAmXUDAeEtrDPaUs?=
 =?us-ascii?Q?ygYC/1uzN1Tbq1luDeUd2pXaDzBKyLeHDzVtwed64NIQc1ocZMK8B7FjClPM?=
 =?us-ascii?Q?ksQPY96CjlgblmuoC3IP2XOcEZIRHIan7kvDPmquIoqVGKNJEkbTnN9MW5i1?=
 =?us-ascii?Q?QGjrWPDln0FVdkz2ajZf62jP2tjCzSYwkfSYBy00UtczM5VE6AISX0T/M6eq?=
 =?us-ascii?Q?Yr4faHi+NDEuMDGpBO16xcVOC5YLmXnOeoDefCrAZNYRticK4/zGzS5aHaCu?=
 =?us-ascii?Q?ov1jC4veQoUQhTkb6k5T+BLIw/e1eFfBDipD4MBvGS7lCNlSlWWziiur6u1+?=
 =?us-ascii?Q?pjXcqRXpgfEyGKr7paEfr2HX/9UtJEhyO2cizLQJ/kV/3Sa2kooukyWrfLH7?=
 =?us-ascii?Q?CI7qK5fWJ7GUj5yBR+QfvxN3PjNLvxRC3r3brcnsVKmAvZVpPBV8IEP7VXqN?=
 =?us-ascii?Q?MTSblc2uUVQ5TrrjT2W5aeMqAoquO64W6g9ODF2rrNcUGrZw+d+zphuyypTc?=
 =?us-ascii?Q?AnN4+unD5l0tTpVB6UntPhj8IFc4YaVlTRXvmhojSrR1PSM3r7o2FULYbVhm?=
 =?us-ascii?Q?7vofybl2ZvO9oDfoWD7O+xe/BTD3+CBmbOnikCs3ZFKXSslQLQxzYiP5ZzBz?=
 =?us-ascii?Q?gFvG1DH/gr0wwTDLKE6GDAbDQGwx8qk+JO/RjXLCk4Z0kHTS0g2ndkDuDmmN?=
 =?us-ascii?Q?2uvh+U6rEmmAkyIGRbXPAv5/ZUT3AFE/Akbp3LeVMTXDvgpI6gNuTcbuaDTj?=
 =?us-ascii?Q?8tMLLycvXWLz+cNoLGfdnkLaShvJNwUBTuAYdSNiqNaOWUMrlk3P37j3Tn5e?=
 =?us-ascii?Q?k9Ik8B17DYu/a2XXu9xP/yu1MJd3jE1+ljxHuEzDjEFCbASb8k4LgYsmLAix?=
 =?us-ascii?Q?NETqo4QOMwYtrWXyZD6tIXaUIZmns13UpekpS9t9fLY6U1/+i/fQJ3EVTtPs?=
 =?us-ascii?Q?6IK2jA/L9plillvpgdDbNgoDqAYtfj9YXabIVWVRjw5FHqS3lB5MMzDKz/kT?=
 =?us-ascii?Q?WDOqJMxlApmG7oy/Sq6SftYKLmH8laySI4jJ2ch+QGd57uImlq1ImcBAjHus?=
 =?us-ascii?Q?rEBQIpAoZJCCaVGmcIBUGg5vomAoypx1YtjkD5tkbyYsMbcvVxIOebWB3uAM?=
 =?us-ascii?Q?ibkYQeUrTMHXwLRZTCd6U72upyql/2LT+FpTtc4EdRpt9jj2X4bEcFRrxy96?=
 =?us-ascii?Q?+QHMD5xYbcczIlTyAKxrMtx4yP6USnn2Fg8hYjVMIhfxWcqbXojKNSbsQfkP?=
 =?us-ascii?Q?BahQKYmXSYpm6nNxviMDIvce2owA2cr7Uac3zn4HatyEG0cBZgHw9X4vKw7u?=
 =?us-ascii?Q?0Bn3PfvzuIA40djvK/eXnuj+MYDEbsMEc0RGiYi9sZot+DLe8LkACpO9hvMw?=
 =?us-ascii?Q?7W4HK2Vriwys3ZecfTksc4WjUUUBFTLj4YpTrRUpCFZthbIhUjEx7XOnQOlC?=
 =?us-ascii?Q?UIti6ynPQcb5FiGwpCg=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6257.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 85e80954-0870-4cc8-1322-08db8c35f7d9
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jul 2023 11:06:02.1903
 (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: JoCIJTTLjMeW3CoNIIkg219rKXCOy1UcvYg0ndGzaWy64uK/MUgMmbrpmwI2Z7IWGSoBifc0+fBul0dsKMd7og==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5218
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> > > +/* helper function to get the number of specific codec */
>=20
> ...and leak a lot of reference counts...
>=20
> > > +static int get_num_codecs(const char *hid) {
> > > +	struct acpi_device *adev =3D NULL;
> > > +	int dev_num =3D 0;
> > > +
> > > +	do {
> > > +		adev =3D acpi_dev_get_next_match_dev(adev, hid, NULL, -1);
> >
> > Humm, I am a bit worried about reference counts.
> >
> > See
> > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L9
> > 16, it's not clear to me where the get() is done.
> >
> > Adding Andy to make sure this is done right.
>=20
> Thank you for Cc'ing.
>=20
> Yes, the above code is problematic. One has to use the respective for_eac=
h macro
> (defined nearby the used API).
>=20
> > > +		if (adev)
> > > +			dev_num++;
> > > +	} while (adev !=3D NULL);
> > > +
> > > +	return dev_num;
> > > +}
>=20
> --
> With Best Regards,
> Andy Shevchenko
>=20
Hi Andy,

Each invocation of acpi_dev_get_next_match_dev() calls acpi_dev_put() to re=
lease the
adev from previous call. And the last call returns NULL. It seems to me the=
 reference count
should be fine. Is my understanding correct?

I saw the macro for_each_acpi_dev_match and re-write the function as follow=
. Thanks for
suggesting using the macro.

/* helper function to get the number of specific codec */
static int get_num_codecs(const char *hid) {
	struct acpi_device *adev;
	int dev_num =3D 0;

	for_each_acpi_dev_match(adev, hid, NULL, -1)
		dev_num++;

	return dev_num;
}

Will test it in next few days.

Regards,
Brent