Re: umass: size for free
Mathieu - wrote: > Mathieu - wrote: > > > > Hello, > > > > Martin Pieuchot wrote: > > > Hello, > > > > > > On 07/12/15(Mon) 16:48, Mathieu - wrote: > > > > Hello, > > > > > > > > I worked a bit on umass(4) recently and had a diff to pass the > > > > umassbus_softc's real size to free so here it is. At some point I > > > > pondered about deleting the whole abstraction, as it would simplify the > > > > free'ing, for we only have one implementation (umass_scsi_softc, as > > > > atapi > > > > uses it too). But I figured it would be against the whole design of the > > > > umass driver, thoughts? > > > > > > I'd rather create a umass_scsi_detach() function symmetrical to > > > umass_scsi_attach(). This way you don't need an extra variable > > > for the size, keep the autoconf(9) glue inside umass_scsi.c and > > > can turn "struct umassbus_softc" into an opaque type. > > > > Please find a patch implementing the suggested approach below. > > Oups I left a debugging printf in there. Here is the clean patch, sorry > again for the noise. Anyone?
Re: umass: size for free
Hello,
On 07/12/15(Mon) 16:48, Mathieu - wrote:
> Hello,
>
> I worked a bit on umass(4) recently and had a diff to pass the
> umassbus_softc's real size to free so here it is. At some point I
> pondered about deleting the whole abstraction, as it would simplify the
> free'ing, for we only have one implementation (umass_scsi_softc, as atapi
> uses it too). But I figured it would be against the whole design of the
> umass driver, thoughts?
I'd rather create a umass_scsi_detach() function symmetrical to
umass_scsi_attach(). This way you don't need an extra variable
for the size, keep the autoconf(9) glue inside umass_scsi.c and
can turn "struct umassbus_softc" into an opaque type.
>
> Index: usb/umass.c
> ===
> RCS file: /cvs/src/sys/dev/usb/umass.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 umass.c
> --- usb/umass.c 14 Mar 2015 03:38:50 - 1.70
> +++ usb/umass.c 7 Dec 2015 15:40:15 -
> @@ -651,7 +651,7 @@ umass_detach(struct device *self, int fl
> if (scbus != NULL) {
> if (scbus->sc_child != NULL)
> rv = config_detach(scbus->sc_child, flags);
> - free(scbus, M_DEVBUF, 0);
> + free(scbus, M_DEVBUF, scbus->sc_size);
> sc->bus = NULL;
> }
>
> Index: usb/umass_scsi.c
> ===
> RCS file: /cvs/src/sys/dev/usb/umass_scsi.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 umass_scsi.c
> --- usb/umass_scsi.c 14 Mar 2015 03:38:50 - 1.42
> +++ usb/umass_scsi.c 7 Dec 2015 15:40:16 -
> @@ -145,6 +145,7 @@ umass_scsi_setup(struct umass_softc *sc)
> struct umass_scsi_softc *scbus;
>
> scbus = malloc(sizeof(*scbus), M_DEVBUF, M_WAITOK | M_ZERO);
> + scbus->base.sc_size = sizeof(*scbus);
>
> sc->bus = (struct umassbus_softc *)scbus;
>
> Index: usb/umassvar.h
> ===
> RCS file: /cvs/src/sys/dev/usb/umassvar.h,v
> retrieving revision 1.14
> diff -u -p -r1.14 umassvar.h
> --- usb/umassvar.h6 Nov 2013 14:37:31 - 1.14
> +++ usb/umassvar.h7 Dec 2015 15:40:16 -
> @@ -146,6 +146,7 @@ struct umass_wire_methods {
>
> struct umassbus_softc {
> struct device *sc_child; /* child device, for detach */
> + size_t sc_size;
> };
>
> /* the per device structure */
>
Re: umass: size for free
Mathieu - wrote:
>
> Hello,
>
> Martin Pieuchot wrote:
> > Hello,
> >
> > On 07/12/15(Mon) 16:48, Mathieu - wrote:
> > > Hello,
> > >
> > > I worked a bit on umass(4) recently and had a diff to pass the
> > > umassbus_softc's real size to free so here it is. At some point I
> > > pondered about deleting the whole abstraction, as it would simplify the
> > > free'ing, for we only have one implementation (umass_scsi_softc, as atapi
> > > uses it too). But I figured it would be against the whole design of the
> > > umass driver, thoughts?
> >
> > I'd rather create a umass_scsi_detach() function symmetrical to
> > umass_scsi_attach(). This way you don't need an extra variable
> > for the size, keep the autoconf(9) glue inside umass_scsi.c and
> > can turn "struct umassbus_softc" into an opaque type.
>
> Please find a patch implementing the suggested approach below.
Oups I left a debugging printf in there. Here is the clean patch, sorry
again for the noise.
Index: usb/umass.c
===
RCS file: /cvs/src/sys/dev/usb/umass.c,v
retrieving revision 1.70
diff -u -p -r1.70 umass.c
--- usb/umass.c 14 Mar 2015 03:38:50 - 1.70
+++ usb/umass.c 10 Dec 2015 11:03:16 -
@@ -129,7 +129,6 @@
#include
#include
#include
-#include
#include
#include
#undef KASSERT
@@ -616,7 +615,6 @@ int
umass_detach(struct device *self, int flags)
{
struct umass_softc *sc = (struct umass_softc *)self;
- struct umassbus_softc *scbus;
int rv = 0, i, s;
DPRINTF(UDMASS_USB, ("%s: detached\n", sc->sc_dev.dv_xname));
@@ -647,12 +645,16 @@ umass_detach(struct device *self, int fl
}
splx(s);
- scbus = sc->bus;
- if (scbus != NULL) {
- if (scbus->sc_child != NULL)
- rv = config_detach(scbus->sc_child, flags);
- free(scbus, M_DEVBUF, 0);
- sc->bus = NULL;
+ switch (sc->sc_cmd) {
+ case UMASS_CPROTO_RBC:
+ case UMASS_CPROTO_SCSI:
+ rv = umass_scsi_detach(sc, flags);
+ break;
+
+ case UMASS_CPROTO_UFI:
+ case UMASS_CPROTO_ATAPI:
+ rv = umass_atapi_detach(sc, flags);
+ break;
}
if (rv != 0)
Index: usb/umass_scsi.c
===
RCS file: /cvs/src/sys/dev/usb/umass_scsi.c,v
retrieving revision 1.42
diff -u -p -r1.42 umass_scsi.c
--- usb/umass_scsi.c14 Mar 2015 03:38:50 - 1.42
+++ usb/umass_scsi.c10 Dec 2015 11:03:17 -
@@ -51,8 +51,8 @@
#include
#include
-struct umass_scsi_softc {
- struct umassbus_softc base;
+struct umassbus_softc {
+ struct device *sc_child; /* child device, for detach */
struct scsi_linksc_link;
struct scsi_iopool sc_iopool;
int sc_open;
@@ -78,7 +78,7 @@ void umass_scsi_cb(struct umass_softc *s
int status);
void umass_scsi_sense_cb(struct umass_softc *sc, void *priv, int residue,
int status);
-struct umass_scsi_softc *umass_scsi_setup(struct umass_softc *);
+struct umassbus_softc *umass_scsi_setup(struct umass_softc *);
void *umass_io_get(void *);
void umass_io_put(void *, void *);
@@ -87,7 +87,7 @@ int
umass_scsi_attach(struct umass_softc *sc)
{
struct scsibus_attach_args saa;
- struct umass_scsi_softc *scbus;
+ struct umassbus_softc *scbus;
scbus = umass_scsi_setup(sc);
scbus->sc_link.adapter_target = UMASS_SCSIID_HOST;
@@ -103,8 +103,7 @@ umass_scsi_attach(struct umass_softc *sc
sc->sc_dev.dv_xname, sc, scbus));
sc->sc_refcnt++;
- scbus->base.sc_child =
- config_found((struct device *)sc, , scsiprint);
+ scbus->sc_child = config_found((struct device *)sc, , scsiprint);
if (--sc->sc_refcnt < 0)
usb_detach_wakeup(>sc_dev);
@@ -115,7 +114,7 @@ int
umass_atapi_attach(struct umass_softc *sc)
{
struct scsibus_attach_args saa;
- struct umass_scsi_softc *scbus;
+ struct umassbus_softc *scbus;
scbus = umass_scsi_setup(sc);
scbus->sc_link.adapter_target = UMASS_SCSIID_HOST;
@@ -131,22 +130,21 @@ umass_atapi_attach(struct umass_softc *s
sc->sc_dev.dv_xname, sc, scbus));
sc->sc_refcnt++;
- scbus->base.sc_child = config_found((struct device *)sc,
- , scsiprint);
+ scbus->sc_child = config_found((struct device *)sc, , scsiprint);
if (--sc->sc_refcnt < 0)
usb_detach_wakeup(>sc_dev);
return (0);
}
-struct umass_scsi_softc *
+struct umassbus_softc *
umass_scsi_setup(struct umass_softc *sc)
{
- struct umass_scsi_softc *scbus;
+ struct umassbus_softc *scbus;
scbus = malloc(sizeof(*scbus), M_DEVBUF, M_WAITOK | M_ZERO);
- sc->bus = (struct
Re: umass: size for free
Hello,
Martin Pieuchot wrote:
> Hello,
>
> On 07/12/15(Mon) 16:48, Mathieu - wrote:
> > Hello,
> >
> > I worked a bit on umass(4) recently and had a diff to pass the
> > umassbus_softc's real size to free so here it is. At some point I
> > pondered about deleting the whole abstraction, as it would simplify the
> > free'ing, for we only have one implementation (umass_scsi_softc, as atapi
> > uses it too). But I figured it would be against the whole design of the
> > umass driver, thoughts?
>
> I'd rather create a umass_scsi_detach() function symmetrical to
> umass_scsi_attach(). This way you don't need an extra variable
> for the size, keep the autoconf(9) glue inside umass_scsi.c and
> can turn "struct umassbus_softc" into an opaque type.
Please find a patch implementing the suggested approach below.
Index: usb/umass.c
===
RCS file: /cvs/src/sys/dev/usb/umass.c,v
retrieving revision 1.70
diff -u -p -r1.70 umass.c
--- usb/umass.c 14 Mar 2015 03:38:50 - 1.70
+++ usb/umass.c 10 Dec 2015 10:57:43 -
@@ -129,7 +129,6 @@
#include
#include
#include
-#include
#include
#include
#undef KASSERT
@@ -616,7 +615,6 @@ int
umass_detach(struct device *self, int flags)
{
struct umass_softc *sc = (struct umass_softc *)self;
- struct umassbus_softc *scbus;
int rv = 0, i, s;
DPRINTF(UDMASS_USB, ("%s: detached\n", sc->sc_dev.dv_xname));
@@ -647,12 +645,16 @@ umass_detach(struct device *self, int fl
}
splx(s);
- scbus = sc->bus;
- if (scbus != NULL) {
- if (scbus->sc_child != NULL)
- rv = config_detach(scbus->sc_child, flags);
- free(scbus, M_DEVBUF, 0);
- sc->bus = NULL;
+ switch (sc->sc_cmd) {
+ case UMASS_CPROTO_RBC:
+ case UMASS_CPROTO_SCSI:
+ rv = umass_scsi_detach(sc, flags);
+ break;
+
+ case UMASS_CPROTO_UFI:
+ case UMASS_CPROTO_ATAPI:
+ rv = umass_atapi_detach(sc, flags);
+ break;
}
if (rv != 0)
Index: usb/umass_scsi.c
===
RCS file: /cvs/src/sys/dev/usb/umass_scsi.c,v
retrieving revision 1.42
diff -u -p -r1.42 umass_scsi.c
--- usb/umass_scsi.c14 Mar 2015 03:38:50 - 1.42
+++ usb/umass_scsi.c10 Dec 2015 10:57:43 -
@@ -51,8 +51,8 @@
#include
#include
-struct umass_scsi_softc {
- struct umassbus_softc base;
+struct umassbus_softc {
+ struct device *sc_child; /* child device, for detach */
struct scsi_linksc_link;
struct scsi_iopool sc_iopool;
int sc_open;
@@ -78,7 +78,7 @@ void umass_scsi_cb(struct umass_softc *s
int status);
void umass_scsi_sense_cb(struct umass_softc *sc, void *priv, int residue,
int status);
-struct umass_scsi_softc *umass_scsi_setup(struct umass_softc *);
+struct umassbus_softc *umass_scsi_setup(struct umass_softc *);
void *umass_io_get(void *);
void umass_io_put(void *, void *);
@@ -87,7 +87,7 @@ int
umass_scsi_attach(struct umass_softc *sc)
{
struct scsibus_attach_args saa;
- struct umass_scsi_softc *scbus;
+ struct umassbus_softc *scbus;
scbus = umass_scsi_setup(sc);
scbus->sc_link.adapter_target = UMASS_SCSIID_HOST;
@@ -103,8 +103,7 @@ umass_scsi_attach(struct umass_softc *sc
sc->sc_dev.dv_xname, sc, scbus));
sc->sc_refcnt++;
- scbus->base.sc_child =
- config_found((struct device *)sc, , scsiprint);
+ scbus->sc_child = config_found((struct device *)sc, , scsiprint);
if (--sc->sc_refcnt < 0)
usb_detach_wakeup(>sc_dev);
@@ -115,7 +114,7 @@ int
umass_atapi_attach(struct umass_softc *sc)
{
struct scsibus_attach_args saa;
- struct umass_scsi_softc *scbus;
+ struct umassbus_softc *scbus;
scbus = umass_scsi_setup(sc);
scbus->sc_link.adapter_target = UMASS_SCSIID_HOST;
@@ -131,22 +130,21 @@ umass_atapi_attach(struct umass_softc *s
sc->sc_dev.dv_xname, sc, scbus));
sc->sc_refcnt++;
- scbus->base.sc_child = config_found((struct device *)sc,
- , scsiprint);
+ scbus->sc_child = config_found((struct device *)sc, , scsiprint);
if (--sc->sc_refcnt < 0)
usb_detach_wakeup(>sc_dev);
return (0);
}
-struct umass_scsi_softc *
+struct umassbus_softc *
umass_scsi_setup(struct umass_softc *sc)
{
- struct umass_scsi_softc *scbus;
+ struct umassbus_softc *scbus;
scbus = malloc(sizeof(*scbus), M_DEVBUF, M_WAITOK | M_ZERO);
- sc->bus = (struct umassbus_softc *)scbus;
+ sc->bus = scbus;
scsi_iopool_init(>sc_iopool, scbus, umass_io_get, umass_io_put);
@@ -162,6 +160,30 @@
umass: size for free
Hello,
I worked a bit on umass(4) recently and had a diff to pass the
umassbus_softc's real size to free so here it is. At some point I
pondered about deleting the whole abstraction, as it would simplify the
free'ing, for we only have one implementation (umass_scsi_softc, as atapi
uses it too). But I figured it would be against the whole design of the
umass driver, thoughts?
Index: usb/umass.c
===
RCS file: /cvs/src/sys/dev/usb/umass.c,v
retrieving revision 1.70
diff -u -p -r1.70 umass.c
--- usb/umass.c 14 Mar 2015 03:38:50 - 1.70
+++ usb/umass.c 7 Dec 2015 15:40:15 -
@@ -651,7 +651,7 @@ umass_detach(struct device *self, int fl
if (scbus != NULL) {
if (scbus->sc_child != NULL)
rv = config_detach(scbus->sc_child, flags);
- free(scbus, M_DEVBUF, 0);
+ free(scbus, M_DEVBUF, scbus->sc_size);
sc->bus = NULL;
}
Index: usb/umass_scsi.c
===
RCS file: /cvs/src/sys/dev/usb/umass_scsi.c,v
retrieving revision 1.42
diff -u -p -r1.42 umass_scsi.c
--- usb/umass_scsi.c14 Mar 2015 03:38:50 - 1.42
+++ usb/umass_scsi.c7 Dec 2015 15:40:16 -
@@ -145,6 +145,7 @@ umass_scsi_setup(struct umass_softc *sc)
struct umass_scsi_softc *scbus;
scbus = malloc(sizeof(*scbus), M_DEVBUF, M_WAITOK | M_ZERO);
+ scbus->base.sc_size = sizeof(*scbus);
sc->bus = (struct umassbus_softc *)scbus;
Index: usb/umassvar.h
===
RCS file: /cvs/src/sys/dev/usb/umassvar.h,v
retrieving revision 1.14
diff -u -p -r1.14 umassvar.h
--- usb/umassvar.h 6 Nov 2013 14:37:31 - 1.14
+++ usb/umassvar.h 7 Dec 2015 15:40:16 -
@@ -146,6 +146,7 @@ struct umass_wire_methods {
struct umassbus_softc {
struct device *sc_child; /* child device, for detach */
+ size_t sc_size;
};
/* the per device structure */
