| Message ID | 1415010853-4334-1-git-send-email-sudipm.mukherjee@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Takashi Iwai |
| Headers | show |
At Mon, 3 Nov 2014 16:04:12 +0530, Sudip Mukherjee wrote: > > added reference of struct echoaudio to free_firmware function. > this structure will be later used to get a reference of the card > when converting snd_printk to dev_* in the next patch of the series. > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> Thanks, now applied both. Though, I noticed that there is also a dev_notice() usage that should be also dev_dbg(). Corrected such lines in my side. Takashi
On Mon, 2014-11-03 at 15:17 +0100, Takashi Iwai wrote: > At Mon, 3 Nov 2014 16:04:12 +0530, > Sudip Mukherjee wrote: > > > > added reference of struct echoaudio to free_firmware function. > > this structure will be later used to get a reference of the card > > when converting snd_printk to dev_* in the next patch of the series. > > > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> > > Thanks, now applied both. Though, I noticed that there is also a > dev_notice() usage that should be also dev_dbg(). Corrected such > lines in my side. Are any of these changes going to cause a null pointer dereference of chip->card->dev? ftrace exists and is generic. Several of these seem to be function tracing style uses and should just be deleted instead.
At Mon, 03 Nov 2014 07:08:08 -0800, Joe Perches wrote: > > On Mon, 2014-11-03 at 15:17 +0100, Takashi Iwai wrote: > > At Mon, 3 Nov 2014 16:04:12 +0530, > > Sudip Mukherjee wrote: > > > > > > added reference of struct echoaudio to free_firmware function. > > > this structure will be later used to get a reference of the card > > > when converting snd_printk to dev_* in the next patch of the series. > > > > > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> > > > > Thanks, now applied both. Though, I noticed that there is also a > > dev_notice() usage that should be also dev_dbg(). Corrected such > > lines in my side. > > Are any of these changes going to cause a > null pointer dereference of chip->card->dev? All look OK at a quick glance. There was a recent change to make card->dev mandatory, so it's there from the very beginning. > ftrace exists and is generic. > > Several of these seem to be function tracing > style uses and should just be deleted instead. True. Further cleanup patches appreciated. thanks, Takashi
On Mon, Nov 03, 2014 at 07:08:08AM -0800, Joe Perches wrote: > On Mon, 2014-11-03 at 15:17 +0100, Takashi Iwai wrote: > > At Mon, 3 Nov 2014 16:04:12 +0530, > > Sudip Mukherjee wrote: > > > > > > added reference of struct echoaudio to free_firmware function. > > > this structure will be later used to get a reference of the card > > > when converting snd_printk to dev_* in the next patch of the series. > > > > > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> > > > > Thanks, now applied both. Though, I noticed that there is also a > > dev_notice() usage that should be also dev_dbg(). Corrected such > > lines in my side. > > Are any of these changes going to cause a > null pointer dereference of chip->card->dev? should not. while converting i have checked the place from where the particular function is called, to see if chip is valid there. if chip is valid, then chip->card->dev should be ok. card is coming from snd_card_new and chip from snd_echo_create. > > ftrace exists and is generic. > > Several of these seem to be function tracing > style uses and should just be deleted instead. function tracing style uses means? there are some prints which are printing the function name while printing the debug message. are those function tracing style uses?? thanks sudip > >
On Mon, 2014-11-03 at 22:22 +0530, Sudip Mukherjee wrote: > On Mon, Nov 03, 2014 at 07:08:08AM -0800, Joe Perches wrote: [] > > ftrace exists and is generic. > > > > Several of these seem to be function tracing > > style uses and should just be deleted instead. > > function tracing style uses means? ftrace see: Documentation/trace/ftrace.txt Documentation/trace/ftrace-design.txt > there are some prints which are printing the function name while printing the debug message. > are those function tracing style uses?? A debug message should generally not be emitting a message just for the sake of documenting when a function is entered/left. Sometimes that is useful for initial debugging, but almost always those debugging printks should not be left in the code that is pushed to kernel.org.
diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c index d82321f..db1b247 100644 --- a/sound/pci/echoaudio/echoaudio.c +++ b/sound/pci/echoaudio/echoaudio.c @@ -69,7 +69,8 @@ static int get_firmware(const struct firmware **fw_entry, -static void free_firmware(const struct firmware *fw_entry) +static void free_firmware(const struct firmware *fw_entry, + struct echoaudio *chip) { #ifdef CONFIG_PM_SLEEP DE_ACT(("firmware not released (kept in cache)\n")); diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h index b86b88d..a4f112a 100644 --- a/sound/pci/echoaudio/echoaudio.h +++ b/sound/pci/echoaudio/echoaudio.h @@ -468,7 +468,8 @@ static int wait_handshake(struct echoaudio *chip); static int send_vector(struct echoaudio *chip, u32 command); static int get_firmware(const struct firmware **fw_entry, struct echoaudio *chip, const short fw_index); -static void free_firmware(const struct firmware *fw_entry); +static void free_firmware(const struct firmware *fw_entry, + struct echoaudio *chip); #ifdef ECHOCARD_HAS_MIDI static int enable_midi_input(struct echoaudio *chip, char enable); diff --git a/sound/pci/echoaudio/echoaudio_dsp.c b/sound/pci/echoaudio/echoaudio_dsp.c index 5a6a217..977b2bd 100644 --- a/sound/pci/echoaudio/echoaudio_dsp.c +++ b/sound/pci/echoaudio/echoaudio_dsp.c @@ -206,12 +206,12 @@ static int load_asic_generic(struct echoaudio *chip, u32 cmd, short asic) } DE_INIT(("ASIC loaded\n")); - free_firmware(fw); + free_firmware(fw, chip); return 0; la_error: DE_INIT(("failed on write_dsp\n")); - free_firmware(fw); + free_firmware(fw, chip); return -EIO; } @@ -317,11 +317,11 @@ static int install_resident_loader(struct echoaudio *chip) } DE_INIT(("Resident loader successfully installed\n")); - free_firmware(fw); + free_firmware(fw, chip); return 0; irl_error: - free_firmware(fw); + free_firmware(fw, chip); return -EIO; } @@ -491,7 +491,7 @@ static int load_firmware(struct echoaudio *chip) if (err < 0) return err; err = load_dsp(chip, (u16 *)fw->data); - free_firmware(fw); + free_firmware(fw, chip); if (err < 0) return err;
added reference of struct echoaudio to free_firmware function. this structure will be later used to get a reference of the card when converting snd_printk to dev_* in the next patch of the series. Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> --- same as v1. sound/pci/echoaudio/echoaudio.c | 3 ++- sound/pci/echoaudio/echoaudio.h | 3 ++- sound/pci/echoaudio/echoaudio_dsp.c | 10 +++++----- 3 files changed, 9 insertions(+), 7 deletions(-)