* Re: Optiarc DVD RW AD-5200A audio playing [not found] <47B5EDB7.5050709@canonical.com> @ 2008-02-16 9:05 ` Borislav Petkov 2008-02-16 15:24 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Borislav Petkov @ 2008-02-16 9:05 UTC (permalink / raw) To: Stefan Bader; +Cc: bzolnier, linux-kernel, linux-ide On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > Hello Borislav, > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > which obviously has the same problem as some Matshita drives. The > following patch was reported to enabled audio playing on this drive. > Would this approach be suitable for upstream or are there other > solutions to this problem? > > Regards, > Stefan > > --- a/drivers/ide/ide-cd.c > +++ b/drivers/ide/ide-cd.c > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > #if ! STANDARD_ATAPI Hi Stefan, just to make sure that the audioplay bit is not set in the capabilities page, can you please try the following patch applied against 2.6.25-rc2 and send me the output. Thanks! @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double negation to check whether a feature is supported or not. Yeah, this comes from "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags or something similar instead, which mirrors the caps page bits setting? commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 Author: Borislav Petkov <petkovbb@gmail.com> Date: Sat Feb 16 09:56:36 2008 +0100 ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive Reported-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Borislav Petkov <petkovbb@gmail.com> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c index f77db6b..2c9d06e 100644 --- a/drivers/ide/ide-cd.c +++ b/drivers/ide/ide-cd.c @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); if (buf[8 + 3] & 0x10) cdi->mask &= ~CDC_DVD_R; + if (!(buf[8 + 4] & 0x01)) { + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," + " enabling quirk.\n"); + } if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) cdi->mask &= ~CDC_PLAY_AUDIO; @@ -1929,6 +1933,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { NULL, NULL, 0 } }; -- Regards/Gruß, Boris. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 9:05 ` Optiarc DVD RW AD-5200A audio playing Borislav Petkov @ 2008-02-16 15:24 ` Bartlomiej Zolnierkiewicz 2008-02-16 16:43 ` Borislav Petkov 0 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2008-02-16 15:24 UTC (permalink / raw) To: petkovbb; +Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe Hi, On Saturday 16 February 2008, Borislav Petkov wrote: > On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > Hello Borislav, > > > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > which obviously has the same problem as some Matshita drives. The > > following patch was reported to enabled audio playing on this drive. > > Would this approach be suitable for upstream or are there other > > solutions to this problem? > > > > Regards, > > Stefan > > > > --- a/drivers/ide/ide-cd.c > > +++ b/drivers/ide/ide-cd.c > > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > > > #if ! STANDARD_ATAPI > > Hi Stefan, > > just to make sure that the audioplay bit is not set in the capabilities page, > can you please try the following patch applied against 2.6.25-rc2 and send me > the output. Thanks! > > @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > negation to check whether a feature is supported or not. Yeah, this comes from > "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > or something similar instead, which mirrors the caps page bits setting? It seems so (at least having negative flags is very unintuitive) but they might be some reason for this ugliness, Jens? [ Please also remember that since cdrom layer is _uniform_ it may be not possible and/or desirable to have 1-1 mapping between caps page bits and the future cdi->caps_flags. ] > commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > Author: Borislav Petkov <petkovbb@gmail.com> > Date: Sat Feb 16 09:56:36 2008 +0100 > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > index f77db6b..2c9d06e 100644 > --- a/drivers/ide/ide-cd.c > +++ b/drivers/ide/ide-cd.c > @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > if (buf[8 + 3] & 0x10) > cdi->mask &= ~CDC_DVD_R; > + if (!(buf[8 + 4] & 0x01)) { Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' to prevent false positives? > + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," Would be nice to also printk() the device name. > + " enabling quirk.\n"); > + } > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > cdi->mask &= ~CDC_PLAY_AUDIO; > > @@ -1929,6 +1933,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { NULL, NULL, 0 } > }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 15:24 ` Bartlomiej Zolnierkiewicz @ 2008-02-16 16:43 ` Borislav Petkov 2008-02-16 17:48 ` Bartlomiej Zolnierkiewicz 2008-02-18 14:17 ` Stefan Bader 0 siblings, 2 replies; 13+ messages in thread From: Borislav Petkov @ 2008-02-16 16:43 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > On Saturday 16 February 2008, Borislav Petkov wrote: > > On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > > Hello Borislav, > > > > > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > > which obviously has the same problem as some Matshita drives. The > > > following patch was reported to enabled audio playing on this drive. > > > Would this approach be suitable for upstream or are there other > > > solutions to this problem? > > > > > > Regards, > > > Stefan > > > > > > --- a/drivers/ide/ide-cd.c > > > +++ b/drivers/ide/ide-cd.c > > > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > > > > > #if ! STANDARD_ATAPI > > > > Hi Stefan, > > > > just to make sure that the audioplay bit is not set in the capabilities page, > > can you please try the following patch applied against 2.6.25-rc2 and send me > > the output. Thanks! > > > > @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > > negation to check whether a feature is supported or not. Yeah, this comes from > > "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > > or something similar instead, which mirrors the caps page bits setting? > > It seems so (at least having negative flags is very unintuitive) but they > might be some reason for this ugliness, Jens? > > [ Please also remember that since cdrom layer is _uniform_ it may be not > possible and/or desirable to have 1-1 mapping between caps page bits > and the future cdi->caps_flags. ] > > > commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > > Author: Borislav Petkov <petkovbb@gmail.com> > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > index f77db6b..2c9d06e 100644 > > --- a/drivers/ide/ide-cd.c > > +++ b/drivers/ide/ide-cd.c > > @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > if (buf[8 + 3] & 0x10) > > cdi->mask &= ~CDC_DVD_R; > > + if (!(buf[8 + 4] & 0x01)) { > > Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > to prevent false positives? I wanted to see whether the caps page reports the audioplay bit off... > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > > Would be nice to also printk() the device name. ... but printing the device model is actually a good idea and this will rule out false positives, so Stefan, please drop the previous patch and test the updated one below. Thanks. commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 Author: Borislav Petkov <petkovbb@gmail.com> Date: Sat Feb 16 09:56:36 2008 +0100 ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive Reported-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Borislav Petkov <petkovbb@gmail.com> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c index f77db6b..4c9984f 100644 --- a/drivers/ide/ide-cd.c +++ b/drivers/ide/ide-cd.c @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); if (buf[8 + 3] & 0x10) cdi->mask &= ~CDC_DVD_R; + if (!(buf[8 + 4] & 0x01)) { + printk(KERN_INFO "ide-cd: audio play not advertised in caps " + "page for drive model [%s], enabling quirk.\n", + drive->id->model); + } if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) cdi->mask &= ~CDC_PLAY_AUDIO; @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { NULL, NULL, 0 } }; -- Regards/Gruß, Boris. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 16:43 ` Borislav Petkov @ 2008-02-16 17:48 ` Bartlomiej Zolnierkiewicz 2008-02-16 18:04 ` Borislav Petkov 2008-02-18 14:17 ` Stefan Bader 1 sibling, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2008-02-16 17:48 UTC (permalink / raw) To: petkovbb; +Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Saturday 16 February 2008, Borislav Petkov wrote: > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > Hi, > > > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > > > Hello Borislav, > > > > > > > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > > > which obviously has the same problem as some Matshita drives. The > > > > following patch was reported to enabled audio playing on this drive. > > > > Would this approach be suitable for upstream or are there other > > > > solutions to this problem? > > > > > > > > Regards, > > > > Stefan > > > > > > > > --- a/drivers/ide/ide-cd.c > > > > +++ b/drivers/ide/ide-cd.c > > > > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > > > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > > > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > > > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > > > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > > > > > > > #if ! STANDARD_ATAPI > > > > > > Hi Stefan, > > > > > > just to make sure that the audioplay bit is not set in the capabilities page, > > > can you please try the following patch applied against 2.6.25-rc2 and send me > > > the output. Thanks! > > > > > > @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > > > negation to check whether a feature is supported or not. Yeah, this comes from > > > "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > > > or something similar instead, which mirrors the caps page bits setting? > > > > It seems so (at least having negative flags is very unintuitive) but they > > might be some reason for this ugliness, Jens? > > > > [ Please also remember that since cdrom layer is _uniform_ it may be not > > possible and/or desirable to have 1-1 mapping between caps page bits > > and the future cdi->caps_flags. ] > > > > > commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > index f77db6b..2c9d06e 100644 > > > --- a/drivers/ide/ide-cd.c > > > +++ b/drivers/ide/ide-cd.c > > > @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > if (buf[8 + 3] & 0x10) > > > cdi->mask &= ~CDC_DVD_R; > > > + if (!(buf[8 + 4] & 0x01)) { > > > > Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > > to prevent false positives? > > I wanted to see whether the caps page reports the audioplay bit off... we still need IDE_CD_FLAG_PLAY_AUDIO_OK flag to be _set_ to enable the quirk > > > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > > > > Would be nice to also printk() the device name. > > ... but printing the device model is actually a good idea and this will rule out > false positives, so Stefan, please drop the previous patch and test the updated > one below. Thanks. > > > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > Author: Borislav Petkov <petkovbb@gmail.com> > Date: Sat Feb 16 09:56:36 2008 +0100 > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > index f77db6b..4c9984f 100644 > --- a/drivers/ide/ide-cd.c > +++ b/drivers/ide/ide-cd.c > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > if (buf[8 + 3] & 0x10) > cdi->mask &= ~CDC_DVD_R; > + if (!(buf[8 + 4] & 0x01)) { > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > + "page for drive model [%s], enabling quirk.\n", > + drive->id->model); if IDE_CD_FLAG_PLAY_AUDIO_OK flag is not set the above message is _bogus_ (because the quirk won't be enabled) [ how's about just deleting the whole printk() to preserve simplicity? ] > + } > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > cdi->mask &= ~CDC_PLAY_AUDIO; > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { NULL, NULL, 0 } > }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 17:48 ` Bartlomiej Zolnierkiewicz @ 2008-02-16 18:04 ` Borislav Petkov 2008-02-16 18:23 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Borislav Petkov @ 2008-02-16 18:04 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Sat, Feb 16, 2008 at 06:48:24PM +0100, Bartlomiej Zolnierkiewicz wrote: > On Saturday 16 February 2008, Borislav Petkov wrote: > > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > > > Hi, > > > > > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > > On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > > > > Hello Borislav, > > > > > > > > > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > > > > which obviously has the same problem as some Matshita drives. The > > > > > following patch was reported to enabled audio playing on this drive. > > > > > Would this approach be suitable for upstream or are there other > > > > > solutions to this problem? > > > > > > > > > > Regards, > > > > > Stefan > > > > > > > > > > --- a/drivers/ide/ide-cd.c > > > > > +++ b/drivers/ide/ide-cd.c > > > > > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > > > > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > > > > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > > > > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > > > > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > > > > > > > > > #if ! STANDARD_ATAPI > > > > > > > > Hi Stefan, > > > > > > > > just to make sure that the audioplay bit is not set in the capabilities page, > > > > can you please try the following patch applied against 2.6.25-rc2 and send me > > > > the output. Thanks! > > > > > > > > @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > > > > negation to check whether a feature is supported or not. Yeah, this comes from > > > > "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > > > > or something similar instead, which mirrors the caps page bits setting? > > > > > > It seems so (at least having negative flags is very unintuitive) but they > > > might be some reason for this ugliness, Jens? > > > > > > [ Please also remember that since cdrom layer is _uniform_ it may be not > > > possible and/or desirable to have 1-1 mapping between caps page bits > > > and the future cdi->caps_flags. ] > > > > > > > commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > > index f77db6b..2c9d06e 100644 > > > > --- a/drivers/ide/ide-cd.c > > > > +++ b/drivers/ide/ide-cd.c > > > > @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > > if (buf[8 + 3] & 0x10) > > > > cdi->mask &= ~CDC_DVD_R; > > > > + if (!(buf[8 + 4] & 0x01)) { > > > > > > Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > > > to prevent false positives? > > > > I wanted to see whether the caps page reports the audioplay bit off... > > we still need IDE_CD_FLAG_PLAY_AUDIO_OK flag to be _set_ to enable the quirk > > > > > > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > > > > > > Would be nice to also printk() the device name. > > > > ... but printing the device model is actually a good idea and this will rule out > > false positives, so Stefan, please drop the previous patch and test the updated > > one below. Thanks. > > > > > > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > > Author: Borislav Petkov <petkovbb@gmail.com> > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > index f77db6b..4c9984f 100644 > > --- a/drivers/ide/ide-cd.c > > +++ b/drivers/ide/ide-cd.c > > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > if (buf[8 + 3] & 0x10) > > cdi->mask &= ~CDC_DVD_R; > > + if (!(buf[8 + 4] & 0x01)) { > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > > + "page for drive model [%s], enabling quirk.\n", > > + drive->id->model); > > if IDE_CD_FLAG_PLAY_AUDIO_OK flag is not set the above message is _bogus_ > (because the quirk won't be enabled) > > [ how's about just deleting the whole printk() to preserve simplicity? ] > > > + } > > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > > cdi->mask &= ~CDC_PLAY_AUDIO; > > > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, But the flag _is_ set because the drive is added to the quirk list (line above ^) and ide_cdrom_setup() calls ide_cd_flags() to set the proper flags for cd->cd_flags and later on ide_cdrom_probe_capabilities() works on the same cd->cd_flags for testing IDE_CD_FLAG_PLAY_AUDIO_OK, or have i gone blind completely...? > > { NULL, NULL, 0 } > > }; -- Regards/Gruß, Boris. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 18:04 ` Borislav Petkov @ 2008-02-16 18:23 ` Bartlomiej Zolnierkiewicz 2008-02-16 18:41 ` Borislav Petkov 0 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2008-02-16 18:23 UTC (permalink / raw) To: petkovbb; +Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Saturday 16 February 2008, Borislav Petkov wrote: > On Sat, Feb 16, 2008 at 06:48:24PM +0100, Bartlomiej Zolnierkiewicz wrote: > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > > > > > Hi, > > > > > > > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > > > On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > > > > > Hello Borislav, > > > > > > > > > > > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > > > > > which obviously has the same problem as some Matshita drives. The > > > > > > following patch was reported to enabled audio playing on this drive. > > > > > > Would this approach be suitable for upstream or are there other > > > > > > solutions to this problem? > > > > > > > > > > > > Regards, > > > > > > Stefan > > > > > > > > > > > > --- a/drivers/ide/ide-cd.c > > > > > > +++ b/drivers/ide/ide-cd.c > > > > > > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > > > > > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > > > > > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > > > > > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > > > > > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > > > > > > > > > > > #if ! STANDARD_ATAPI > > > > > > > > > > Hi Stefan, > > > > > > > > > > just to make sure that the audioplay bit is not set in the capabilities page, > > > > > can you please try the following patch applied against 2.6.25-rc2 and send me > > > > > the output. Thanks! > > > > > > > > > > @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > > > > > negation to check whether a feature is supported or not. Yeah, this comes from > > > > > "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > > > > > or something similar instead, which mirrors the caps page bits setting? > > > > > > > > It seems so (at least having negative flags is very unintuitive) but they > > > > might be some reason for this ugliness, Jens? > > > > > > > > [ Please also remember that since cdrom layer is _uniform_ it may be not > > > > possible and/or desirable to have 1-1 mapping between caps page bits > > > > and the future cdi->caps_flags. ] > > > > > > > > > commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > > > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > > > index f77db6b..2c9d06e 100644 > > > > > --- a/drivers/ide/ide-cd.c > > > > > +++ b/drivers/ide/ide-cd.c > > > > > @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > > > if (buf[8 + 3] & 0x10) > > > > > cdi->mask &= ~CDC_DVD_R; > > > > > + if (!(buf[8 + 4] & 0x01)) { > > > > > > > > Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > > > > to prevent false positives? > > > > > > I wanted to see whether the caps page reports the audioplay bit off... > > > > we still need IDE_CD_FLAG_PLAY_AUDIO_OK flag to be _set_ to enable the quirk > > > > > > > > > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > > > > > > > > Would be nice to also printk() the device name. > > > > > > ... but printing the device model is actually a good idea and this will rule out > > > false positives, so Stefan, please drop the previous patch and test the updated > > > one below. Thanks. > > > > > > > > > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > index f77db6b..4c9984f 100644 > > > --- a/drivers/ide/ide-cd.c > > > +++ b/drivers/ide/ide-cd.c > > > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > if (buf[8 + 3] & 0x10) > > > cdi->mask &= ~CDC_DVD_R; > > > + if (!(buf[8 + 4] & 0x01)) { > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > > > + "page for drive model [%s], enabling quirk.\n", > > > + drive->id->model); > > > > if IDE_CD_FLAG_PLAY_AUDIO_OK flag is not set the above message is _bogus_ > > (because the quirk won't be enabled) > > > > [ how's about just deleting the whole printk() to preserve simplicity? ] > > > > > + } > > > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > > > cdi->mask &= ~CDC_PLAY_AUDIO; > > > > > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > > > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > But the flag _is_ set because the drive is added to the quirk list (line above ^) and ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (fortunately) majority of ATAPI drives are not in the quirk list ;-) [ and they may have (buf[8 + 4] & 0x01) == 0 ] > ide_cdrom_setup() calls ide_cd_flags() to set the proper flags for cd->cd_flags and later on > ide_cdrom_probe_capabilities() works on the same cd->cd_flags for testing > IDE_CD_FLAG_PLAY_AUDIO_OK, or have i gone blind completely...? > > > > { NULL, NULL, 0 } > > > }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 18:23 ` Bartlomiej Zolnierkiewicz @ 2008-02-16 18:41 ` Borislav Petkov 2008-02-16 19:20 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Borislav Petkov @ 2008-02-16 18:41 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Sat, Feb 16, 2008 at 07:23:58PM +0100, Bartlomiej Zolnierkiewicz wrote: > On Saturday 16 February 2008, Borislav Petkov wrote: > > On Sat, Feb 16, 2008 at 06:48:24PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > > > > On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > > > > > > Hello Borislav, > > > > > > > > > > > > > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > > > > > > which obviously has the same problem as some Matshita drives. The > > > > > > > following patch was reported to enabled audio playing on this drive. > > > > > > > Would this approach be suitable for upstream or are there other > > > > > > > solutions to this problem? > > > > > > > > > > > > > > Regards, > > > > > > > Stefan > > > > > > > > > > > > > > --- a/drivers/ide/ide-cd.c > > > > > > > +++ b/drivers/ide/ide-cd.c > > > > > > > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > > > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > > > > > > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > > > > > > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > > > > > > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > > > > > > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > > > > > > > > > > > > > #if ! STANDARD_ATAPI > > > > > > > > > > > > Hi Stefan, > > > > > > > > > > > > just to make sure that the audioplay bit is not set in the capabilities page, > > > > > > can you please try the following patch applied against 2.6.25-rc2 and send me > > > > > > the output. Thanks! > > > > > > > > > > > > @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > > > > > > negation to check whether a feature is supported or not. Yeah, this comes from > > > > > > "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > > > > > > or something similar instead, which mirrors the caps page bits setting? > > > > > > > > > > It seems so (at least having negative flags is very unintuitive) but they > > > > > might be some reason for this ugliness, Jens? > > > > > > > > > > [ Please also remember that since cdrom layer is _uniform_ it may be not > > > > > possible and/or desirable to have 1-1 mapping between caps page bits > > > > > and the future cdi->caps_flags. ] > > > > > > > > > > > commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > > > > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > > > > index f77db6b..2c9d06e 100644 > > > > > > --- a/drivers/ide/ide-cd.c > > > > > > +++ b/drivers/ide/ide-cd.c > > > > > > @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > > > > if (buf[8 + 3] & 0x10) > > > > > > cdi->mask &= ~CDC_DVD_R; > > > > > > + if (!(buf[8 + 4] & 0x01)) { > > > > > > > > > > Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > > > > > to prevent false positives? > > > > > > > > I wanted to see whether the caps page reports the audioplay bit off... > > > > > > we still need IDE_CD_FLAG_PLAY_AUDIO_OK flag to be _set_ to enable the quirk > > > > > > > > > > > > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > > > > > > > > > > Would be nice to also printk() the device name. > > > > > > > > ... but printing the device model is actually a good idea and this will rule out > > > > false positives, so Stefan, please drop the previous patch and test the updated > > > > one below. Thanks. > > > > > > > > > > > > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > > index f77db6b..4c9984f 100644 > > > > --- a/drivers/ide/ide-cd.c > > > > +++ b/drivers/ide/ide-cd.c > > > > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > > if (buf[8 + 3] & 0x10) > > > > cdi->mask &= ~CDC_DVD_R; > > > > + if (!(buf[8 + 4] & 0x01)) { > > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > > > > + "page for drive model [%s], enabling quirk.\n", > > > > + drive->id->model); > > > > > > if IDE_CD_FLAG_PLAY_AUDIO_OK flag is not set the above message is _bogus_ > > > (because the quirk won't be enabled) > > > > > > [ how's about just deleting the whole printk() to preserve simplicity? ] > > > > > > > + } > > > > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > > > > cdi->mask &= ~CDC_PLAY_AUDIO; > > > > > > > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > > > > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > But the flag _is_ set because the drive is added to the quirk list (line above ^) and > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > (fortunately) majority of ATAPI drives are not in the quirk list ;-) > > [ and they may have (buf[8 + 4] & 0x01) == 0 ] Bart, i think i probably didn't express myself clear enough before. The patch serves the sole purpose to show exactly whether buf[8 + 4] & 0x01) == 0 and is in no way the final solution. I just wanted to make sure the caps page doesn't set the audioplay bit. The final solution would be to add the drive to the quirks list only, i guess, in case you don't have a better idea. Sorry for the misunderstanding :=). > > > ide_cdrom_setup() calls ide_cd_flags() to set the proper flags for cd->cd_flags and later on > > ide_cdrom_probe_capabilities() works on the same cd->cd_flags for testing > > IDE_CD_FLAG_PLAY_AUDIO_OK, or have i gone blind completely...? > > > > > > { NULL, NULL, 0 } > > > > }; -- Regards/Gruß, Boris. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 18:41 ` Borislav Petkov @ 2008-02-16 19:20 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2008-02-16 19:20 UTC (permalink / raw) To: petkovbb; +Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Saturday 16 February 2008, Borislav Petkov wrote: > On Sat, Feb 16, 2008 at 07:23:58PM +0100, Bartlomiej Zolnierkiewicz wrote: > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > On Sat, Feb 16, 2008 at 06:48:24PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > > > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Saturday 16 February 2008, Borislav Petkov wrote: > > > > > > > On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > > > > > > > Hello Borislav, > > > > > > > > > > > > > > > > I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > > > > > > > which obviously has the same problem as some Matshita drives. The > > > > > > > > following patch was reported to enabled audio playing on this drive. > > > > > > > > Would this approach be suitable for upstream or are there other > > > > > > > > solutions to this problem? > > > > > > > > > > > > > > > > Regards, > > > > > > > > Stefan > > > > > > > > > > > > > > > > --- a/drivers/ide/ide-cd.c > > > > > > > > +++ b/drivers/ide/ide-cd.c > > > > > > > > @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > > > > if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > > > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > > > > > > > strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > > > > > > > - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > > > > > > > + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > > > > > > > + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > > > > > > > CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > > > > > > > > > > > > > > > #if ! STANDARD_ATAPI > > > > > > > > > > > > > > Hi Stefan, > > > > > > > > > > > > > > just to make sure that the audioplay bit is not set in the capabilities page, > > > > > > > can you please try the following patch applied against 2.6.25-rc2 and send me > > > > > > > the output. Thanks! > > > > > > > > > > > > > > @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > > > > > > > negation to check whether a feature is supported or not. Yeah, this comes from > > > > > > > "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > > > > > > > or something similar instead, which mirrors the caps page bits setting? > > > > > > > > > > > > It seems so (at least having negative flags is very unintuitive) but they > > > > > > might be some reason for this ugliness, Jens? > > > > > > > > > > > > [ Please also remember that since cdrom layer is _uniform_ it may be not > > > > > > possible and/or desirable to have 1-1 mapping between caps page bits > > > > > > and the future cdi->caps_flags. ] > > > > > > > > > > > > > commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > > > > > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > > > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > > > > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > > > > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > > > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > > > > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > > > > > index f77db6b..2c9d06e 100644 > > > > > > > --- a/drivers/ide/ide-cd.c > > > > > > > +++ b/drivers/ide/ide-cd.c > > > > > > > @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > > > > > if (buf[8 + 3] & 0x10) > > > > > > > cdi->mask &= ~CDC_DVD_R; > > > > > > > + if (!(buf[8 + 4] & 0x01)) { > > > > > > > > > > > > Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > > > > > > to prevent false positives? > > > > > > > > > > I wanted to see whether the caps page reports the audioplay bit off... > > > > > > > > we still need IDE_CD_FLAG_PLAY_AUDIO_OK flag to be _set_ to enable the quirk > > > > > > > > > > > > > > > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > > > > > > > > > > > > Would be nice to also printk() the device name. > > > > > > > > > > ... but printing the device model is actually a good idea and this will rule out > > > > > false positives, so Stefan, please drop the previous patch and test the updated > > > > > one below. Thanks. > > > > > > > > > > > > > > > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > > > > > Author: Borislav Petkov <petkovbb@gmail.com> > > > > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > > > > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > > > > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > > > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > > > > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > > > > index f77db6b..4c9984f 100644 > > > > > --- a/drivers/ide/ide-cd.c > > > > > +++ b/drivers/ide/ide-cd.c > > > > > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > > > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > > > > if (buf[8 + 3] & 0x10) > > > > > cdi->mask &= ~CDC_DVD_R; > > > > > + if (!(buf[8 + 4] & 0x01)) { > > > > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > > > > > + "page for drive model [%s], enabling quirk.\n", > > > > > + drive->id->model); > > > > > > > > if IDE_CD_FLAG_PLAY_AUDIO_OK flag is not set the above message is _bogus_ > > > > (because the quirk won't be enabled) > > > > > > > > [ how's about just deleting the whole printk() to preserve simplicity? ] > > > > > > > > > + } > > > > > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > > > > > cdi->mask &= ~CDC_PLAY_AUDIO; > > > > > > > > > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > > > > > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > > > > > But the flag _is_ set because the drive is added to the quirk list (line above ^) and > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > (fortunately) majority of ATAPI drives are not in the quirk list ;-) > > > > [ and they may have (buf[8 + 4] & 0x01) == 0 ] > > Bart, > > i think i probably didn't express myself clear enough before. The patch > serves the sole purpose to show exactly whether buf[8 + 4] & 0x01) == 0 and > is in no way the final solution. I just wanted to make sure the caps page > doesn't set the audioplay bit. The final solution would be to add the drive > to the quirks list only, i guess, in case you don't have a better idea. > > Sorry for the misunderstanding :=). Doh, all the time I thought that it is the final patch to be merged. :-) [ I like to merge patches ASAP so please give some hint like putting "Bart: DO NOT APPLY!" after "---" for draft/testing patches. Thanks. ] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-16 16:43 ` Borislav Petkov @ 2008-02-18 14:17 ` Stefan Bader 2008-02-18 14:17 ` Stefan Bader 1 sibling, 0 replies; 13+ messages in thread From: Stefan Bader @ 2008-02-18 14:17 UTC (permalink / raw) To: petkovbb, Bartlomiej Zolnierkiewicz, Stefan Bader, linux-kernel, linux-ide, Jens Axboe <jen> Borislav Petkov wrote: > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: >> Hi, >> >> On Saturday 16 February 2008, Borislav Petkov wrote: >>> On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: >>>> Hello Borislav, >>>> >>>> I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) >>>> which obviously has the same problem as some Matshita drives. The >>>> following patch was reported to enabled audio playing on this drive. >>>> Would this approach be suitable for upstream or are there other >>>> solutions to this problem? >>>> >>>> Regards, >>>> Stefan >>>> >>>> --- a/drivers/ide/ide-cd.c >>>> +++ b/drivers/ide/ide-cd.c >>>> @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) >>>> if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || >>>> - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) >>>> + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || >>>> + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) >>>> CDROM_CONFIG_FLAGS(drive)->audio_play = 1; >>>> >>>> #if ! STANDARD_ATAPI >>> Hi Stefan, >>> >>> just to make sure that the audioplay bit is not set in the capabilities page, >>> can you please try the following patch applied against 2.6.25-rc2 and send me >>> the output. Thanks! >>> >>> @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double >>> negation to check whether a feature is supported or not. Yeah, this comes from >>> "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags >>> or something similar instead, which mirrors the caps page bits setting? >> It seems so (at least having negative flags is very unintuitive) but they >> might be some reason for this ugliness, Jens? >> >> [ Please also remember that since cdrom layer is _uniform_ it may be not >> possible and/or desirable to have 1-1 mapping between caps page bits >> and the future cdi->caps_flags. ] >> >>> commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 >>> Author: Borislav Petkov <petkovbb@gmail.com> >>> Date: Sat Feb 16 09:56:36 2008 +0100 >>> >>> ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive >>> >>> Reported-by: Stefan Bader <stefan.bader@canonical.com> >>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com> >>> >>> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c >>> index f77db6b..2c9d06e 100644 >>> --- a/drivers/ide/ide-cd.c >>> +++ b/drivers/ide/ide-cd.c >>> @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) >>> cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); >>> if (buf[8 + 3] & 0x10) >>> cdi->mask &= ~CDC_DVD_R; >>> + if (!(buf[8 + 4] & 0x01)) { >> Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' >> to prevent false positives? > > I wanted to see whether the caps page reports the audioplay bit off... > >>> + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," >> Would be nice to also printk() the device name. > > ... but printing the device model is actually a good idea and this will rule out > false positives, so Stefan, please drop the previous patch and test the updated > one below. Thanks. > > Hi Borislav, the problem is that I don't own this drive myself and the owner is running a 2.6.22 kernel and is normally not doing any kernel compiles. But I could provide him a modified patch. Though, if you just want to know whether the cap bit was really unset, I think we know this already. When I got the problem report we checked /proc/sys/dev/cdrom/info and that showed the "Can play audio" bit as 0. Which is the reason I gave the owner the patch for adding the model to the excemption list. And from his feedback I take that the drive plays audio tracks with the patch in use. Stefan > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > Author: Borislav Petkov <petkovbb@gmail.com> > Date: Sat Feb 16 09:56:36 2008 +0100 > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > index f77db6b..4c9984f 100644 > --- a/drivers/ide/ide-cd.c > +++ b/drivers/ide/ide-cd.c > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > if (buf[8 + 3] & 0x10) > cdi->mask &= ~CDC_DVD_R; > + if (!(buf[8 + 4] & 0x01)) { > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > + "page for drive model [%s], enabling quirk.\n", > + drive->id->model); > + } > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > cdi->mask &= ~CDC_PLAY_AUDIO; > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { NULL, NULL, 0 } > }; > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing @ 2008-02-18 14:17 ` Stefan Bader 0 siblings, 0 replies; 13+ messages in thread From: Stefan Bader @ 2008-02-18 14:17 UTC (permalink / raw) To: petkovbb, Bartlomiej Zolnierkiewicz, Stefan Bader, linux-kernel, linux-ide, Jens Axboe Borislav Petkov wrote: > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: >> Hi, >> >> On Saturday 16 February 2008, Borislav Petkov wrote: >>> On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: >>>> Hello Borislav, >>>> >>>> I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) >>>> which obviously has the same problem as some Matshita drives. The >>>> following patch was reported to enabled audio playing on this drive. >>>> Would this approach be suitable for upstream or are there other >>>> solutions to this problem? >>>> >>>> Regards, >>>> Stefan >>>> >>>> --- a/drivers/ide/ide-cd.c >>>> +++ b/drivers/ide/ide-cd.c >>>> @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) >>>> if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || >>>> - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) >>>> + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || >>>> + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) >>>> CDROM_CONFIG_FLAGS(drive)->audio_play = 1; >>>> >>>> #if ! STANDARD_ATAPI >>> Hi Stefan, >>> >>> just to make sure that the audioplay bit is not set in the capabilities page, >>> can you please try the following patch applied against 2.6.25-rc2 and send me >>> the output. Thanks! >>> >>> @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double >>> negation to check whether a feature is supported or not. Yeah, this comes from >>> "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags >>> or something similar instead, which mirrors the caps page bits setting? >> It seems so (at least having negative flags is very unintuitive) but they >> might be some reason for this ugliness, Jens? >> >> [ Please also remember that since cdrom layer is _uniform_ it may be not >> possible and/or desirable to have 1-1 mapping between caps page bits >> and the future cdi->caps_flags. ] >> >>> commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 >>> Author: Borislav Petkov <petkovbb@gmail.com> >>> Date: Sat Feb 16 09:56:36 2008 +0100 >>> >>> ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive >>> >>> Reported-by: Stefan Bader <stefan.bader@canonical.com> >>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com> >>> >>> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c >>> index f77db6b..2c9d06e 100644 >>> --- a/drivers/ide/ide-cd.c >>> +++ b/drivers/ide/ide-cd.c >>> @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) >>> cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); >>> if (buf[8 + 3] & 0x10) >>> cdi->mask &= ~CDC_DVD_R; >>> + if (!(buf[8 + 4] & 0x01)) { >> Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' >> to prevent false positives? > > I wanted to see whether the caps page reports the audioplay bit off... > >>> + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," >> Would be nice to also printk() the device name. > > ... but printing the device model is actually a good idea and this will rule out > false positives, so Stefan, please drop the previous patch and test the updated > one below. Thanks. > > Hi Borislav, the problem is that I don't own this drive myself and the owner is running a 2.6.22 kernel and is normally not doing any kernel compiles. But I could provide him a modified patch. Though, if you just want to know whether the cap bit was really unset, I think we know this already. When I got the problem report we checked /proc/sys/dev/cdrom/info and that showed the "Can play audio" bit as 0. Which is the reason I gave the owner the patch for adding the model to the excemption list. And from his feedback I take that the drive plays audio tracks with the patch in use. Stefan > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > Author: Borislav Petkov <petkovbb@gmail.com> > Date: Sat Feb 16 09:56:36 2008 +0100 > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > index f77db6b..4c9984f 100644 > --- a/drivers/ide/ide-cd.c > +++ b/drivers/ide/ide-cd.c > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > if (buf[8 + 3] & 0x10) > cdi->mask &= ~CDC_DVD_R; > + if (!(buf[8 + 4] & 0x01)) { > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > + "page for drive model [%s], enabling quirk.\n", > + drive->id->model); > + } > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > cdi->mask &= ~CDC_PLAY_AUDIO; > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > { NULL, NULL, 0 } > }; > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-18 14:17 ` Stefan Bader (?) @ 2008-02-18 23:18 ` Bartlomiej Zolnierkiewicz 2008-02-19 5:52 ` Borislav Petkov -1 siblings, 1 reply; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2008-02-18 23:18 UTC (permalink / raw) To: Stefan Bader; +Cc: petkovbb, linux-kernel, linux-ide, Jens Axboe On Monday 18 February 2008, Stefan Bader wrote: > Borislav Petkov wrote: > > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > >> Hi, > >> > >> On Saturday 16 February 2008, Borislav Petkov wrote: > >>> On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > >>>> Hello Borislav, > >>>> > >>>> I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > >>>> which obviously has the same problem as some Matshita drives. The > >>>> following patch was reported to enabled audio playing on this drive. > >>>> Would this approach be suitable for upstream or are there other > >>>> solutions to this problem? > >>>> > >>>> Regards, > >>>> Stefan > >>>> > >>>> --- a/drivers/ide/ide-cd.c > >>>> +++ b/drivers/ide/ide-cd.c > >>>> @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > >>>> if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > >>>> - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > >>>> + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > >>>> + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > >>>> CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > >>>> > >>>> #if ! STANDARD_ATAPI > >>> Hi Stefan, > >>> > >>> just to make sure that the audioplay bit is not set in the capabilities page, > >>> can you please try the following patch applied against 2.6.25-rc2 and send me > >>> the output. Thanks! > >>> > >>> @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > >>> negation to check whether a feature is supported or not. Yeah, this comes from > >>> "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > >>> or something similar instead, which mirrors the caps page bits setting? > >> It seems so (at least having negative flags is very unintuitive) but they > >> might be some reason for this ugliness, Jens? > >> > >> [ Please also remember that since cdrom layer is _uniform_ it may be not > >> possible and/or desirable to have 1-1 mapping between caps page bits > >> and the future cdi->caps_flags. ] > >> > >>> commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > >>> Author: Borislav Petkov <petkovbb@gmail.com> > >>> Date: Sat Feb 16 09:56:36 2008 +0100 > >>> > >>> ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > >>> > >>> Reported-by: Stefan Bader <stefan.bader@canonical.com> > >>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > >>> > >>> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > >>> index f77db6b..2c9d06e 100644 > >>> --- a/drivers/ide/ide-cd.c > >>> +++ b/drivers/ide/ide-cd.c > >>> @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > >>> cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > >>> if (buf[8 + 3] & 0x10) > >>> cdi->mask &= ~CDC_DVD_R; > >>> + if (!(buf[8 + 4] & 0x01)) { > >> Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > >> to prevent false positives? > > > > I wanted to see whether the caps page reports the audioplay bit off... > > > >>> + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > >> Would be nice to also printk() the device name. > > > > ... but printing the device model is actually a good idea and this will rule out > > false positives, so Stefan, please drop the previous patch and test the updated > > one below. Thanks. > > > > > > Hi Borislav, > > the problem is that I don't own this drive myself and the owner is > running a 2.6.22 kernel and is normally not doing any kernel compiles. > But I could provide him a modified patch. > Though, if you just want to know whether the cap bit was really unset, I > think we know this already. When I got the problem report we checked > /proc/sys/dev/cdrom/info and that showed the "Can play audio" bit as 0. > Which is the reason I gave the owner the patch for adding the model to > the excemption list. And from his feedback I take that the drive plays > audio tracks with the patch in use. Borislav, I guess that this is good enough proof that audioplay bit is off. Could you please send me the final version of the patch? > Stefan > > > commit 6cc44b0ce5c9270b15d456eb9ffa91b855e4e0d0 > > Author: Borislav Petkov <petkovbb@gmail.com> > > Date: Sat Feb 16 09:56:36 2008 +0100 > > > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > > > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > index f77db6b..4c9984f 100644 > > --- a/drivers/ide/ide-cd.c > > +++ b/drivers/ide/ide-cd.c > > @@ -1750,6 +1750,11 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > if (buf[8 + 3] & 0x10) > > cdi->mask &= ~CDC_DVD_R; > > + if (!(buf[8 + 4] & 0x01)) { > > + printk(KERN_INFO "ide-cd: audio play not advertised in caps " > > + "page for drive model [%s], enabling quirk.\n", > > + drive->id->model); > > + } > > if ((buf[8 + 4] & 0x01) || (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)) > > cdi->mask &= ~CDC_PLAY_AUDIO; > > > > @@ -1929,6 +1934,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { > > { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, > > { NULL, NULL, 0 } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-18 23:18 ` Bartlomiej Zolnierkiewicz @ 2008-02-19 5:52 ` Borislav Petkov 2008-02-19 22:10 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 13+ messages in thread From: Borislav Petkov @ 2008-02-19 5:52 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Tue, Feb 19, 2008 at 12:18:48AM +0100, Bartlomiej Zolnierkiewicz wrote: > On Monday 18 February 2008, Stefan Bader wrote: > > Borislav Petkov wrote: > > > On Sat, Feb 16, 2008 at 04:24:01PM +0100, Bartlomiej Zolnierkiewicz wrote: > > >> Hi, > > >> > > >> On Saturday 16 February 2008, Borislav Petkov wrote: > > >>> On Fri, Feb 15, 2008 at 02:53:27PM -0500, Stefan Bader wrote: > > >>>> Hello Borislav, > > >>>> > > >>>> I worked on a problem with an DVD driver (model=Optiarc DVD RW AD-5200A) > > >>>> which obviously has the same problem as some Matshita drives. The > > >>>> following patch was reported to enabled audio playing on this drive. > > >>>> Would this approach be suitable for upstream or are there other > > >>>> solutions to this problem? > > >>>> > > >>>> Regards, > > >>>> Stefan > > >>>> > > >>>> --- a/drivers/ide/ide-cd.c > > >>>> +++ b/drivers/ide/ide-cd.c > > >>>> @@ -2988,7 +2988,8 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > >>>> if (strcmp(drive->id->model, "MATSHITADVD-ROM SR-8187") == 0 || > > >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8186") == 0 || > > >>>> strcmp(drive->id->model, "MATSHITADVD-ROM SR-8176") == 0 || > > >>>> - strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0) > > >>>> + strcmp(drive->id->model, "MATSHITADVD-ROM SR-8174") == 0 || > > >>>> + strcmp(drive->id->model, "Optiarc DVD RW AD-5200A") == 0) > > >>>> CDROM_CONFIG_FLAGS(drive)->audio_play = 1; > > >>>> > > >>>> #if ! STANDARD_ATAPI > > >>> Hi Stefan, > > >>> > > >>> just to make sure that the audioplay bit is not set in the capabilities page, > > >>> can you please try the following patch applied against 2.6.25-rc2 and send me > > >>> the output. Thanks! > > >>> > > >>> @Bart: by the way, this cdi->mask thingy is kinda unintuitive doing double > > >>> negation to check whether a feature is supported or not. Yeah, this comes from > > >>> "above," i.e. uniform cdrom layer. But still, shouldn't we use a cdi->caps_flags > > >>> or something similar instead, which mirrors the caps page bits setting? > > >> It seems so (at least having negative flags is very unintuitive) but they > > >> might be some reason for this ugliness, Jens? > > >> > > >> [ Please also remember that since cdrom layer is _uniform_ it may be not > > >> possible and/or desirable to have 1-1 mapping between caps page bits > > >> and the future cdi->caps_flags. ] > > >> > > >>> commit 435f0f4496a1b32af2d542f43b2370a890fe2f83 > > >>> Author: Borislav Petkov <petkovbb@gmail.com> > > >>> Date: Sat Feb 16 09:56:36 2008 +0100 > > >>> > > >>> ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > >>> > > >>> Reported-by: Stefan Bader <stefan.bader@canonical.com> > > >>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com> > > >>> > > >>> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > > >>> index f77db6b..2c9d06e 100644 > > >>> --- a/drivers/ide/ide-cd.c > > >>> +++ b/drivers/ide/ide-cd.c > > >>> @@ -1750,6 +1750,10 @@ int ide_cdrom_probe_capabilities (ide_drive_t *drive) > > >>> cdi->mask &= ~(CDC_DVD_RAM | CDC_RAM); > > >>> if (buf[8 + 3] & 0x10) > > >>> cdi->mask &= ~CDC_DVD_R; > > >>> + if (!(buf[8 + 4] & 0x01)) { > > >> Hmm, shouldn't there be '&& (cd->cd_flags & IDE_CD_FLAG_PLAY_AUDIO_OK)' > > >> to prevent false positives? > > > > > > I wanted to see whether the caps page reports the audioplay bit off... > > > > > >>> + printk(KERN_INFO "ide-cd: audio play not advertised in caps page," > > >> Would be nice to also printk() the device name. > > > > > > ... but printing the device model is actually a good idea and this will rule out > > > false positives, so Stefan, please drop the previous patch and test the updated > > > one below. Thanks. > > > > > > > > > > Hi Borislav, > > > > the problem is that I don't own this drive myself and the owner is > > running a 2.6.22 kernel and is normally not doing any kernel compiles. > > But I could provide him a modified patch. > > Though, if you just want to know whether the cap bit was really unset, I > > think we know this already. When I got the problem report we checked > > /proc/sys/dev/cdrom/info and that showed the "Can play audio" bit as 0. > > Which is the reason I gave the owner the patch for adding the model to > > the excemption list. And from his feedback I take that the drive plays > > audio tracks with the patch in use. > > Borislav, I guess that this is good enough proof that audioplay bit is off. indeed. > Could you please send me the final version of the patch? commit fa4af2fab0804bead4da6ecbf468118f05111229 Author: Borislav Petkov <petkovbb@gmail.com> Date: Sat Feb 16 09:56:36 2008 +0100 ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive Reported-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Borislav Petkov <petkovbb@gmail.com> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c index f77db6b..9e63c34 100644 --- a/drivers/ide/ide-cd.c +++ b/drivers/ide/ide-cd.c @@ -1929,6 +1929,7 @@ static const struct cd_list_entry ide_cd_quirks_list[] = { { "MATSHITADVD-ROM SR-8186", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { "MATSHITADVD-ROM SR-8176", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { "MATSHITADVD-ROM SR-8174", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, + { "Optiarc DVD RW AD-5200A", NULL, IDE_CD_FLAG_PLAY_AUDIO_OK }, { NULL, NULL, 0 } }; -- Regards/Gruß, Boris. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Optiarc DVD RW AD-5200A audio playing 2008-02-19 5:52 ` Borislav Petkov @ 2008-02-19 22:10 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 13+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2008-02-19 22:10 UTC (permalink / raw) To: petkovbb; +Cc: Stefan Bader, linux-kernel, linux-ide, Jens Axboe On Tuesday 19 February 2008, Borislav Petkov wrote: [...] > Author: Borislav Petkov <petkovbb@gmail.com> > Date: Sat Feb 16 09:56:36 2008 +0100 > > ide-cd: Enable audio play quirk for Optiarc DVD RW AD-5200A drive > > Reported-by: Stefan Bader <stefan.bader@canonical.com> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com> applied ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2008-02-19 22:04 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <47B5EDB7.5050709@canonical.com>
2008-02-16 9:05 ` Optiarc DVD RW AD-5200A audio playing Borislav Petkov
2008-02-16 15:24 ` Bartlomiej Zolnierkiewicz
2008-02-16 16:43 ` Borislav Petkov
2008-02-16 17:48 ` Bartlomiej Zolnierkiewicz
2008-02-16 18:04 ` Borislav Petkov
2008-02-16 18:23 ` Bartlomiej Zolnierkiewicz
2008-02-16 18:41 ` Borislav Petkov
2008-02-16 19:20 ` Bartlomiej Zolnierkiewicz
2008-02-18 14:17 ` Stefan Bader
2008-02-18 14:17 ` Stefan Bader
2008-02-18 23:18 ` Bartlomiej Zolnierkiewicz
2008-02-19 5:52 ` Borislav Petkov
2008-02-19 22:10 ` Bartlomiej Zolnierkiewicz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.