Re: Replace audio(9) get_props() with duplex check in open() in partial-duplex drivers
On Thu, Oct 27, 2022 at 08:35:31PM +, Klemens Nanni wrote:
> On Thu, Oct 27, 2022 at 03:51:05PM +0200, Alexandre Ratchov wrote:
> > On Thu, Oct 27, 2022 at 01:08:57PM +, Klemens Nanni wrote:
> > > @@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags)
> > >
> > > DPRINTF(("ad1848_open: sc=%p\n", sc));
> > >
> > > + if ((flags & (FWRITE | FREAD)) && sc->mode != 2)
> > > + return ENXIO;
> > > +
> > > sc->sc_pintr = sc->sc_parg = NULL;
> > > sc->sc_rintr = sc->sc_rarg = NULL;
> > >
> >
> > afaiu, the correct condition for full-duplex check is:
> >
> > if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD))
> > ...
> >
> > (both FWRITE and FREAD set)
>
> Here's the correct version, sorry for the noise.
no problem! ok ratchov
> ---
> sys/dev/isa/ad1848.c| 12
> sys/dev/isa/ad1848var.h | 2 --
> sys/dev/isa/ess.c | 29 -
> sys/dev/isa/gus.c | 20
> sys/dev/isa/gusvar.h| 2 --
> sys/dev/isa/pas.c | 1 -
> sys/dev/isa/sb.c| 1 -
> sys/dev/isa/sbdsp.c | 11 ---
> sys/dev/isa/sbdspvar.h | 2 --
> 9 files changed, 24 insertions(+), 56 deletions(-)
>
> diff --git a/sys/dev/isa/ad1848.c b/sys/dev/isa/ad1848.c
> index bb2b95277a1..26b2cc2ba88 100644
> --- a/sys/dev/isa/ad1848.c
> +++ b/sys/dev/isa/ad1848.c
> @@ -74,6 +74,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags)
>
> DPRINTF(("ad1848_open: sc=%p\n", sc));
>
> + if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD) && sc->mode != 2)
> + return ENXIO;
> +
> sc->sc_pintr = sc->sc_parg = NULL;
> sc->sc_rintr = sc->sc_rarg = NULL;
>
> @@ -1458,11 +1462,3 @@ ad1848_round(void *addr, int direction, size_t size)
> size = MAX_ISADMA;
> return size;
> }
> -
> -int
> -ad1848_get_props(void *addr)
> -{
> - struct ad1848_softc *sc = addr;
> -
> - return (sc->mode == 2 ? AUDIO_PROP_FULLDUPLEX : 0);
> -}
> diff --git a/sys/dev/isa/ad1848var.h b/sys/dev/isa/ad1848var.h
> index 49c199f64fc..c8af3f04b4b 100644
> --- a/sys/dev/isa/ad1848var.h
> +++ b/sys/dev/isa/ad1848var.h
> @@ -210,6 +210,4 @@ void *ad1848_malloc(void *, int, size_t, int, int);
> void ad1848_free(void *, void *, int);
> size_t ad1848_round(void *, int, size_t);
>
> -int ad1848_get_props(void *);
> -
> #endif
> diff --git a/sys/dev/isa/ess.c b/sys/dev/isa/ess.c
> index 8989dd94b01..eba480013cf 100644
> --- a/sys/dev/isa/ess.c
> +++ b/sys/dev/isa/ess.c
> @@ -74,6 +74,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -115,6 +116,7 @@ struct audio_params ess_audio_default =
>
> int ess_setup_sc(struct ess_softc *, int);
>
> +int ess_1788_open(void *, int);
> int ess_open(void *, int);
> void ess_1788_close(void *);
> void ess_1888_close(void *);
> @@ -148,8 +150,6 @@ size_tess_round_buffersize(void *, int, size_t);
>
>
> int ess_query_devinfo(void *, mixer_devinfo_t *);
> -int ess_1788_get_props(void *);
> -int ess_1888_get_props(void *);
>
> void ess_speaker_on(struct ess_softc *);
> void ess_speaker_off(struct ess_softc *);
> @@ -198,7 +198,7 @@ static const char *essmodel[] = {
> */
>
> const struct audio_hw_if ess_1788_hw_if = {
> - .open = ess_open,
> + .open = ess_1788_open,
> .close = ess_1788_close,
> .set_params = ess_set_params,
> .round_blocksize = ess_round_blocksize,
> @@ -211,7 +211,6 @@ const struct audio_hw_if ess_1788_hw_if = {
> .allocm = ess_malloc,
> .freem = ess_free,
> .round_buffersize = ess_round_buffersize,
> - .get_props = ess_1788_get_props,
> .trigger_output = ess_audio1_trigger_output,
> .trigger_input = ess_audio1_trigger_input,
> };
> @@ -230,7 +229,6 @@ const struct audio_hw_if ess_1888_hw_if = {
> .allocm = ess_malloc,
> .freem = ess_free,
> .round_buffersize = ess_round_buffersize,
> - .get_props = ess_1888_get_props,
> .trigger_output = ess_audio2_trigger_output,
> .trigger_input = ess_audio1_trigger_input,
> };
> @@ -982,6 +980,15 @@ essattach(struct ess_softc *sc)
> * Various routines to interface to higher level audio driver
> */
>
> +int
> +ess_1788_open(void *addr, int flags)
> +{
> + if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD))
> + return ENXIO;
> +
> + return ess_open(addr, flags);
> +}
> +
> int
> ess_open(void *addr, int flags)
> {
> @@ -2059,18 +2066,6 @@ ess_round_buffersize(void *addr, int direction, size_t
> size)
> return (size);
> }
>
> -int
> -ess_1788_get_props(void *addr)
> -{
> - return (0);
> -}
> -
> -int
> -ess_1888_get_props(void *addr)
> -{
> - return (AUDIO_PROP_FULLDUPLEX);
> -}
> -
> /*
> * Generic functions for ess, not used by audio h/w i/f
> *
Re: Replace audio(9) get_props() with duplex check in open() in partial-duplex drivers
On Thu, Oct 27, 2022 at 03:51:05PM +0200, Alexandre Ratchov wrote:
> On Thu, Oct 27, 2022 at 01:08:57PM +, Klemens Nanni wrote:
> > @@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags)
> >
> > DPRINTF(("ad1848_open: sc=%p\n", sc));
> >
> > + if ((flags & (FWRITE | FREAD)) && sc->mode != 2)
> > + return ENXIO;
> > +
> > sc->sc_pintr = sc->sc_parg = NULL;
> > sc->sc_rintr = sc->sc_rarg = NULL;
> >
>
> afaiu, the correct condition for full-duplex check is:
>
> if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD))
> ...
>
> (both FWRITE and FREAD set)
Here's the correct version, sorry for the noise.
Feedback? OK?
---
sys/dev/isa/ad1848.c| 12
sys/dev/isa/ad1848var.h | 2 --
sys/dev/isa/ess.c | 29 -
sys/dev/isa/gus.c | 20
sys/dev/isa/gusvar.h| 2 --
sys/dev/isa/pas.c | 1 -
sys/dev/isa/sb.c| 1 -
sys/dev/isa/sbdsp.c | 11 ---
sys/dev/isa/sbdspvar.h | 2 --
9 files changed, 24 insertions(+), 56 deletions(-)
diff --git a/sys/dev/isa/ad1848.c b/sys/dev/isa/ad1848.c
index bb2b95277a1..26b2cc2ba88 100644
--- a/sys/dev/isa/ad1848.c
+++ b/sys/dev/isa/ad1848.c
@@ -74,6 +74,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags)
DPRINTF(("ad1848_open: sc=%p\n", sc));
+ if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD) && sc->mode != 2)
+ return ENXIO;
+
sc->sc_pintr = sc->sc_parg = NULL;
sc->sc_rintr = sc->sc_rarg = NULL;
@@ -1458,11 +1462,3 @@ ad1848_round(void *addr, int direction, size_t size)
size = MAX_ISADMA;
return size;
}
-
-int
-ad1848_get_props(void *addr)
-{
- struct ad1848_softc *sc = addr;
-
- return (sc->mode == 2 ? AUDIO_PROP_FULLDUPLEX : 0);
-}
diff --git a/sys/dev/isa/ad1848var.h b/sys/dev/isa/ad1848var.h
index 49c199f64fc..c8af3f04b4b 100644
--- a/sys/dev/isa/ad1848var.h
+++ b/sys/dev/isa/ad1848var.h
@@ -210,6 +210,4 @@ void *ad1848_malloc(void *, int, size_t, int, int);
void ad1848_free(void *, void *, int);
size_t ad1848_round(void *, int, size_t);
-intad1848_get_props(void *);
-
#endif
diff --git a/sys/dev/isa/ess.c b/sys/dev/isa/ess.c
index 8989dd94b01..eba480013cf 100644
--- a/sys/dev/isa/ess.c
+++ b/sys/dev/isa/ess.c
@@ -74,6 +74,7 @@
#include
#include
#include
+#include
#include
#include
@@ -115,6 +116,7 @@ struct audio_params ess_audio_default =
intess_setup_sc(struct ess_softc *, int);
+intess_1788_open(void *, int);
intess_open(void *, int);
void ess_1788_close(void *);
void ess_1888_close(void *);
@@ -148,8 +150,6 @@ size_t ess_round_buffersize(void *, int, size_t);
intess_query_devinfo(void *, mixer_devinfo_t *);
-intess_1788_get_props(void *);
-intess_1888_get_props(void *);
void ess_speaker_on(struct ess_softc *);
void ess_speaker_off(struct ess_softc *);
@@ -198,7 +198,7 @@ static const char *essmodel[] = {
*/
const struct audio_hw_if ess_1788_hw_if = {
- .open = ess_open,
+ .open = ess_1788_open,
.close = ess_1788_close,
.set_params = ess_set_params,
.round_blocksize = ess_round_blocksize,
@@ -211,7 +211,6 @@ const struct audio_hw_if ess_1788_hw_if = {
.allocm = ess_malloc,
.freem = ess_free,
.round_buffersize = ess_round_buffersize,
- .get_props = ess_1788_get_props,
.trigger_output = ess_audio1_trigger_output,
.trigger_input = ess_audio1_trigger_input,
};
@@ -230,7 +229,6 @@ const struct audio_hw_if ess_1888_hw_if = {
.allocm = ess_malloc,
.freem = ess_free,
.round_buffersize = ess_round_buffersize,
- .get_props = ess_1888_get_props,
.trigger_output = ess_audio2_trigger_output,
.trigger_input = ess_audio1_trigger_input,
};
@@ -982,6 +980,15 @@ essattach(struct ess_softc *sc)
* Various routines to interface to higher level audio driver
*/
+int
+ess_1788_open(void *addr, int flags)
+{
+ if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD))
+ return ENXIO;
+
+ return ess_open(addr, flags);
+}
+
int
ess_open(void *addr, int flags)
{
@@ -2059,18 +2066,6 @@ ess_round_buffersize(void *addr, int direction, size_t
size)
return (size);
}
-int
-ess_1788_get_props(void *addr)
-{
- return (0);
-}
-
-int
-ess_1888_get_props(void *addr)
-{
- return (AUDIO_PROP_FULLDUPLEX);
-}
-
/*
* Generic functions for ess, not used by audio h/w i/f
* =
diff --git a/sys/dev/isa/gus.c b/sys/dev/isa/gus.c
index 45fe009fc61..6e1d1a00cef 100644
--- a/sys/dev/isa/gus.c
+++ b/sys/dev/isa/gus.c
@@ -277,7 +277,6 @@ const struct audio_hw_if gus_hw_if = {
.allocm = gus_malloc,
.freem = gus_free,
Re: Replace audio(9) get_props() with duplex check in open() in partial-duplex drivers
27 Oct 2022 17:31:45 Miod Vallat : > Wait, this is wrong. All your > > (flags & (FWRITE | FREAD)) > > comparisons must be > > ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD)) > > in order to truly catch full-duplex opens. Otherwise the condition will > always be satisfied since one of FREAD or FWRITE will always be set... Wrong set of sed'ed diffs, sorry. Will resend the right ones later.
Re: Replace audio(9) get_props() with duplex check in open() in partial-duplex drivers
On Thu, Oct 27, 2022 at 01:08:57PM +, Klemens Nanni wrote:
> @@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags)
>
> DPRINTF(("ad1848_open: sc=%p\n", sc));
>
> + if ((flags & (FWRITE | FREAD)) && sc->mode != 2)
> + return ENXIO;
> +
> sc->sc_pintr = sc->sc_parg = NULL;
> sc->sc_rintr = sc->sc_rarg = NULL;
>
afaiu, the correct condition for full-duplex check is:
if ((flags & (FWRITE | FREAD)) == (FWRITE | FREAD))
...
(both FWRITE and FREAD set)
> @@ -982,6 +980,15 @@ essattach(struct ess_softc *sc)
> * Various routines to interface to higher level audio driver
> */
>
> +int
> +ess_1788_open(void *addr, int flags)
> +{
> + if (flags & (FWRITE | FREAD))
> + return ENXIO;
> +
> + return ess_open(addr, flags);
> +}
> +
> int
> ess_open(void *addr, int flags)
> {
> @@ -2059,18 +2066,6 @@ ess_round_buffersize(void *addr, int direction, size_t
> size)
same here.
> @@ -307,6 +305,9 @@ gusopen(void *addr, int flags)
>
> DPRINTF(("gusopen() called\n"));
>
> + if ((flags & (FWRITE | FREAD)) && sc->sc_recdrq == sc->sc_drq)
> + return ENXIO;
> +
> if (sc->sc_flags & GUS_OPEN)
> return EBUSY;
>
ditto
> @@ -2132,6 +2126,8 @@ sbdsp_midi_open(void *addr, int flags, void
> (*iintr)(void *, int),
>
> DPRINTF(("sbdsp_midi_open: sc=%p\n", sc));
>
> + if ((flags & (FWRITE | FREAD)) && !sc->sc_fullduplex)
> + return ENXIO;
ditto
Replace audio(9) get_props() with duplex check in open() in partial-duplex drivers
Make drivers which do *not* adverise AUDIO_PROP_FULLDPLEX return ENXIO
in their open() if full-duplex mode was requested.
This way, sys/dev/audio.c:audio_open() will fail immediately rather than
later through the to-be-removed get_props() check.
This is the first round for drivers with logic in their get_props(),
i.e. those that only support full-duplex mode for specific hardware:
ess(4), gus(4), pas(4) and sb(4)
All of these are i386/GENERIC only and share code through
sys/dev/isa/{ad1848,sbdsp}{.c,var.h} which is not used by any other
architecture/kernel configuration.
i386/GENERIC.MP builds and boots with this diff.
Feedback? Objection? OK?
---
sys/dev/isa/ad1848.c| 12
sys/dev/isa/ad1848var.h | 2 --
sys/dev/isa/ess.c | 29 -
sys/dev/isa/gus.c | 19 +++
sys/dev/isa/gusvar.h| 2 --
sys/dev/isa/pas.c | 1 -
sys/dev/isa/sb.c| 1 -
sys/dev/isa/sbdsp.c | 10 +++---
sys/dev/isa/sbdspvar.h | 2 --
9 files changed, 22 insertions(+), 56 deletions(-)
diff --git a/sys/dev/isa/ad1848.c b/sys/dev/isa/ad1848.c
index bb2b95277a1..843af8f3b0d 100644
--- a/sys/dev/isa/ad1848.c
+++ b/sys/dev/isa/ad1848.c
@@ -74,6 +74,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1040,6 +1041,9 @@ ad1848_open(void *addr, int flags)
DPRINTF(("ad1848_open: sc=%p\n", sc));
+ if ((flags & (FWRITE | FREAD)) && sc->mode != 2)
+ return ENXIO;
+
sc->sc_pintr = sc->sc_parg = NULL;
sc->sc_rintr = sc->sc_rarg = NULL;
@@ -1458,11 +1462,3 @@ ad1848_round(void *addr, int direction, size_t size)
size = MAX_ISADMA;
return size;
}
-
-int
-ad1848_get_props(void *addr)
-{
- struct ad1848_softc *sc = addr;
-
- return (sc->mode == 2 ? AUDIO_PROP_FULLDUPLEX : 0);
-}
diff --git a/sys/dev/isa/ad1848var.h b/sys/dev/isa/ad1848var.h
index 49c199f64fc..c8af3f04b4b 100644
--- a/sys/dev/isa/ad1848var.h
+++ b/sys/dev/isa/ad1848var.h
@@ -210,6 +210,4 @@ void *ad1848_malloc(void *, int, size_t, int, int);
void ad1848_free(void *, void *, int);
size_t ad1848_round(void *, int, size_t);
-intad1848_get_props(void *);
-
#endif
diff --git a/sys/dev/isa/ess.c b/sys/dev/isa/ess.c
index 8989dd94b01..51ecd05f12a 100644
--- a/sys/dev/isa/ess.c
+++ b/sys/dev/isa/ess.c
@@ -74,6 +74,7 @@
#include
#include
#include
+#include
#include
#include
@@ -115,6 +116,7 @@ struct audio_params ess_audio_default =
intess_setup_sc(struct ess_softc *, int);
+intess_1788_open(void *, int);
intess_open(void *, int);
void ess_1788_close(void *);
void ess_1888_close(void *);
@@ -148,8 +150,6 @@ size_t ess_round_buffersize(void *, int, size_t);
intess_query_devinfo(void *, mixer_devinfo_t *);
-intess_1788_get_props(void *);
-intess_1888_get_props(void *);
void ess_speaker_on(struct ess_softc *);
void ess_speaker_off(struct ess_softc *);
@@ -198,7 +198,7 @@ static const char *essmodel[] = {
*/
const struct audio_hw_if ess_1788_hw_if = {
- .open = ess_open,
+ .open = ess_1788_open,
.close = ess_1788_close,
.set_params = ess_set_params,
.round_blocksize = ess_round_blocksize,
@@ -211,7 +211,6 @@ const struct audio_hw_if ess_1788_hw_if = {
.allocm = ess_malloc,
.freem = ess_free,
.round_buffersize = ess_round_buffersize,
- .get_props = ess_1788_get_props,
.trigger_output = ess_audio1_trigger_output,
.trigger_input = ess_audio1_trigger_input,
};
@@ -230,7 +229,6 @@ const struct audio_hw_if ess_1888_hw_if = {
.allocm = ess_malloc,
.freem = ess_free,
.round_buffersize = ess_round_buffersize,
- .get_props = ess_1888_get_props,
.trigger_output = ess_audio2_trigger_output,
.trigger_input = ess_audio1_trigger_input,
};
@@ -982,6 +980,15 @@ essattach(struct ess_softc *sc)
* Various routines to interface to higher level audio driver
*/
+int
+ess_1788_open(void *addr, int flags)
+{
+ if (flags & (FWRITE | FREAD))
+ return ENXIO;
+
+ return ess_open(addr, flags);
+}
+
int
ess_open(void *addr, int flags)
{
@@ -2059,18 +2066,6 @@ ess_round_buffersize(void *addr, int direction, size_t
size)
return (size);
}
-int
-ess_1788_get_props(void *addr)
-{
- return (0);
-}
-
-int
-ess_1888_get_props(void *addr)
-{
- return (AUDIO_PROP_FULLDUPLEX);
-}
-
/*
* Generic functions for ess, not used by audio h/w i/f
* =
diff --git a/sys/dev/isa/gus.c b/sys/dev/isa/gus.c
index 45fe009fc61..8be5d5abe74 100644
--- a/sys/dev/isa/gus.c
+++ b/sys/dev/isa/gus.c
@@ -277,7 +277,6 @@ const struct audio_hw_if gus_hw_if = {
.allocm = gus_malloc,
.freem = gus_free,
.round_buffersize = gus_round,
- .get_props = gus_get_props,
