Date: Tue, 1 Dec 2009 14:30:31 -0700 (MST) From: Scott Long <scottl@samsco.org> To: Jaakko Heinonen <jh@freebsd.org> Cc: scsi@freebsd.org Subject: Re: cd(4) M_WAITOK allocations with periph lock held Message-ID: <20091201142930.I99667@pooker.samsco.org> In-Reply-To: <20091201180524.GB7961@a91-153-117-195.elisa-laajakaista.fi> References: <20091201180524.GB7961@a91-153-117-195.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] How about the attached match instead. It refactors the code so that unlocks aren't needed. Scott On Tue, 1 Dec 2009, Jaakko Heinonen wrote: > > Hi, > > There are some M_WAITOK malloc invocations with periph lock held in > cd(4). Below is a link to a patch which drops the periph lock while > doing those allocations. Comments/review? > > --- > > Drop periph lock while allocating memory with M_WAITOK flag in > cdreportkey(), cdsendkey() and cdreaddvdstructure(). > > PR: kern/130735 > Tested by: Eygene Ryabinkin > > The patch against head: > > http://people.freebsd.org/~jh/patches/scsi_cd-M_WAITOK-fixes.diff > > -- > Jaakko > _______________________________________________ > freebsd-scsi@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-scsi > To unsubscribe, send any mail to "freebsd-scsi-unsubscribe@freebsd.org" > [-- Attachment #2 --] Index: scsi_cd.c =================================================================== RCS file: /usr1/ncvs/src/sys/cam/scsi/scsi_cd.c,v retrieving revision 1.112 diff -u -r1.112 scsi_cd.c --- scsi_cd.c 14 Nov 2009 20:13:38 -0000 1.112 +++ scsi_cd.c 1 Dec 2009 21:28:00 -0000 @@ -2673,12 +2673,10 @@ authinfo = (struct dvd_authinfo *)addr; - cam_periph_lock(periph); if (cmd == DVDIOCREPORTKEY) error = cdreportkey(periph, authinfo); else error = cdsendkey(periph, authinfo); - cam_periph_unlock(periph); break; } case DVDIOCREADSTRUCTURE: { @@ -2686,9 +2684,7 @@ dvdstruct = (struct dvd_struct *)addr; - cam_periph_lock(periph); error = cdreaddvdstructure(periph, dvdstruct); - cam_periph_unlock(periph); break; } @@ -3732,8 +3728,6 @@ databuf = NULL; lba = 0; - ccb = cdgetccb(periph, CAM_PRIORITY_NORMAL); - switch (authinfo->format) { case DVD_REPORT_AGID: length = sizeof(struct scsi_report_key_data_agid); @@ -3759,9 +3753,7 @@ length = 0; break; default: - error = EINVAL; - goto bailout; - break; /* NOTREACHED */ + return (EINVAL); } if (length != 0) { @@ -3769,6 +3761,8 @@ } else databuf = NULL; + cam_periph_lock(periph); + ccb = cdgetccb(periph, CAM_PRIORITY_NORMAL); scsi_report_key(&ccb->csio, /* retries */ 1, @@ -3870,11 +3864,12 @@ break; /* NOTREACHED */ } bailout: + xpt_release_ccb(ccb); + cam_periph_unlock(periph); + if (databuf != NULL) free(databuf, M_DEVBUF); - xpt_release_ccb(ccb); - return(error); } @@ -3889,8 +3884,6 @@ error = 0; databuf = NULL; - ccb = cdgetccb(periph, CAM_PRIORITY_NORMAL); - switch(authinfo->format) { case DVD_SEND_CHALLENGE: { struct scsi_report_key_data_challenge *challenge_data; @@ -3942,11 +3935,12 @@ break; } default: - error = EINVAL; - goto bailout; - break; /* NOTREACHED */ + return (EINVAL); } + cam_periph_lock(periph); + ccb = cdgetccb(periph, CAM_PRIORITY_NORMAL); + scsi_send_key(&ccb->csio, /* retries */ 1, /* cbfcnp */ cddone, @@ -3963,11 +3957,12 @@ bailout: + xpt_release_ccb(ccb); + cam_periph_unlock(periph); + if (databuf != NULL) free(databuf, M_DEVBUF); - xpt_release_ccb(ccb); - return(error); } @@ -3985,8 +3980,6 @@ /* The address is reserved for many of the formats */ address = 0; - ccb = cdgetccb(periph, CAM_PRIORITY_NORMAL); - switch(dvdstruct->format) { case DVD_STRUCT_PHYSICAL: length = sizeof(struct scsi_read_dvd_struct_data_physical); @@ -4004,13 +3997,7 @@ length = sizeof(struct scsi_read_dvd_struct_data_manufacturer); break; case DVD_STRUCT_CMI: - error = ENODEV; - goto bailout; -#ifdef notyet - length = sizeof(struct scsi_read_dvd_struct_data_copy_manage); - address = dvdstruct->address; -#endif - break; /* NOTREACHED */ + return (ENODEV); case DVD_STRUCT_PROTDISCID: length = sizeof(struct scsi_read_dvd_struct_data_prot_discid); break; @@ -4027,21 +4014,9 @@ length = sizeof(struct scsi_read_dvd_struct_data_spare_area); break; case DVD_STRUCT_RMD_LAST: - error = ENODEV; - goto bailout; -#ifdef notyet - length = sizeof(struct scsi_read_dvd_struct_data_rmd_borderout); - address = dvdstruct->address; -#endif - break; /* NOTREACHED */ + return (ENODEV); case DVD_STRUCT_RMD_RMA: - error = ENODEV; - goto bailout; -#ifdef notyet - length = sizeof(struct scsi_read_dvd_struct_data_rmd); - address = dvdstruct->address; -#endif - break; /* NOTREACHED */ + return (ENODEV); case DVD_STRUCT_PRERECORDED: length = sizeof(struct scsi_read_dvd_struct_data_leadin); break; @@ -4049,13 +4024,7 @@ length = sizeof(struct scsi_read_dvd_struct_data_disc_id); break; case DVD_STRUCT_DCB: - error = ENODEV; - goto bailout; -#ifdef notyet - length = sizeof(struct scsi_read_dvd_struct_data_dcb); - address = dvdstruct->address; -#endif - break; /* NOTREACHED */ + return (ENODEV); case DVD_STRUCT_LIST: /* * This is the maximum allocation length for the READ DVD @@ -4067,9 +4036,7 @@ length = 65535; break; default: - error = EINVAL; - goto bailout; - break; /* NOTREACHED */ + return (EINVAL); } if (length != 0) { @@ -4077,6 +4044,9 @@ } else databuf = NULL; + cam_periph_lock(periph); + ccb = cdgetccb(periph, CAM_PRIORITY_NORMAL); + scsi_read_dvd_structure(&ccb->csio, /* retries */ 1, /* cbfcnp */ cddone, @@ -4166,11 +4136,12 @@ } bailout: + xpt_release_ccb(ccb); + cam_periph_unlock(periph); + if (databuf != NULL) free(databuf, M_DEVBUF); - xpt_release_ccb(ccb); - return(error); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20091201142930.I99667>