From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753768AbcENPNq (ORCPT ); Sat, 14 May 2016 11:13:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:50324 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753496AbcENPNp (ORCPT ); Sat, 14 May 2016 11:13:45 -0400 Date: Sat, 14 May 2016 17:13:42 +0200 Message-ID: From: Takashi Iwai To: Robert Jarzmik Cc: "Haojian Zhuang" , "Liam Girdwood" , "Mark Brown" , "Jaroslav Kysela" , "Daniel Mack" , , , , Subject: Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus In-Reply-To: <87twi1hssl.fsf@belgarion.home> References: <1462050939-27940-1-git-send-email-robert.jarzmik@free.fr> <1462050939-27940-3-git-send-email-robert.jarzmik@free.fr> <87twi1hssl.fsf@belgarion.home> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 14 May 2016 11:50:50 +0200, Robert Jarzmik wrote: > >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, > >> + int codec_num) > >> +{ > >> + struct ac97_codec_device codec; > >> + unsigned short vid1, vid2; > >> + int ret; > >> + > >> + codec.dev = *ac97->dev; > >> + codec.num = codec_num; > >> + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); > >> + vid1 = (ret & 0xffff); > >> + if (ret < 0) > >> + return 0; > > > > Hmm. This looks pretty hackish and dangerous. > You mean returning 0 even if the read failed, right ? No, my concern is that it's creating a dummy codec object temporarily on the stack just by copying some fields and calling the ops with it. (And actually the current code may work wrongly because lack of zero-clear of the object.) IMO, a cleaner way would be to define the ops passed with both controller and codec objects as arguments, and pass NULL codec here. Takashi From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus Date: Sat, 14 May 2016 17:13:42 +0200 Message-ID: References: <1462050939-27940-1-git-send-email-robert.jarzmik@free.fr> <1462050939-27940-3-git-send-email-robert.jarzmik@free.fr> <87twi1hssl.fsf@belgarion.home> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: <87twi1hssl.fsf@belgarion.home> Sender: linux-kernel-owner@vger.kernel.org To: Robert Jarzmik Cc: Haojian Zhuang , Liam Girdwood , Mark Brown , Jaroslav Kysela , Daniel Mack , alsa-devel@alsa-project.org, linux-arm-kernel@lists.infradead.org, patches@opensource.wolfsonmicro.com, linux-kernel@vger.kernel.org List-Id: alsa-devel@alsa-project.org On Sat, 14 May 2016 11:50:50 +0200, Robert Jarzmik wrote: > >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, > >> + int codec_num) > >> +{ > >> + struct ac97_codec_device codec; > >> + unsigned short vid1, vid2; > >> + int ret; > >> + > >> + codec.dev = *ac97->dev; > >> + codec.num = codec_num; > >> + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); > >> + vid1 = (ret & 0xffff); > >> + if (ret < 0) > >> + return 0; > > > > Hmm. This looks pretty hackish and dangerous. > You mean returning 0 even if the read failed, right ? No, my concern is that it's creating a dummy codec object temporarily on the stack just by copying some fields and calling the ops with it. (And actually the current code may work wrongly because lack of zero-clear of the object.) IMO, a cleaner way would be to define the ops passed with both controller and codec objects as arguments, and pass NULL codec here. Takashi From mboxrd@z Thu Jan 1 00:00:00 1970 From: tiwai@suse.de (Takashi Iwai) Date: Sat, 14 May 2016 17:13:42 +0200 Subject: [RFC PATCH 2/7] ALSA: ac97: add an ac97 bus In-Reply-To: <87twi1hssl.fsf@belgarion.home> References: <1462050939-27940-1-git-send-email-robert.jarzmik@free.fr> <1462050939-27940-3-git-send-email-robert.jarzmik@free.fr> <87twi1hssl.fsf@belgarion.home> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sat, 14 May 2016 11:50:50 +0200, Robert Jarzmik wrote: > >> +unsigned int ac97_bus_scan_one(struct ac97_controller *ac97, > >> + int codec_num) > >> +{ > >> + struct ac97_codec_device codec; > >> + unsigned short vid1, vid2; > >> + int ret; > >> + > >> + codec.dev = *ac97->dev; > >> + codec.num = codec_num; > >> + ret = ac97->ops->read(&codec, AC97_VENDOR_ID1); > >> + vid1 = (ret & 0xffff); > >> + if (ret < 0) > >> + return 0; > > > > Hmm. This looks pretty hackish and dangerous. > You mean returning 0 even if the read failed, right ? No, my concern is that it's creating a dummy codec object temporarily on the stack just by copying some fields and calling the ops with it. (And actually the current code may work wrongly because lack of zero-clear of the object.) IMO, a cleaner way would be to define the ops passed with both controller and codec objects as arguments, and pass NULL codec here. Takashi