| Message ID | 170250854466.1522182.17555361077409628655.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | driver core: Add a guard() definition for the device_lock() | expand |
Dan Williams wrote: > At present there are ~200 usages of device_lock() in the kernel. Some of > those usages lead to "goto unlock;" patterns which have proven to be > error prone. Define a "device" guard() definition to allow for those to > be cleaned up and prevent new ones from appearing. > > Link: http://lore.kernel.org/r/657897453dda8_269bd29492@dwillia2-mobl3.amr.corp.intel.com.notmuch > Link: http://lore.kernel.org/r/6577b0c2a02df_a04c5294bb@dwillia2-xfh.jf.intel.com.notmuch > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > Hi Greg, > > I wonder if you might include this change in v6.7-rc to ease some patch > sets alternately going through my tree and Andrew's tree. Those > discussions are linked above. Alternately I can can just take it through > my tree with your ack and the other use case can circle back to it in > the v6.9 cycle. > > I considered also defining a __free() helper similar to __free(mutex), > but I think "__free(device)" would be a surprising name for something > that drops a lock. Also, I like the syntax of guard(device) over > something like guard(device_lock) since a 'struct device *' is the > argument, not a lock type, but I'm open to your or Peter's thoughts on > the naming. > > include/linux/device.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/device.h b/include/linux/device.h > index d7a72a8749ea..6c83294395ac 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -1007,6 +1007,8 @@ static inline void device_unlock(struct device *dev) > mutex_unlock(&dev->mutex); > } > > +DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T)) > + > static inline void device_lock_assert(struct device *dev) > { > lockdep_assert_held(&dev->mutex); >
On Wed, 2023-12-13 at 15:02 -0800, Dan Williams wrote: > At present there are ~200 usages of device_lock() in the kernel. Some of > those usages lead to "goto unlock;" patterns which have proven to be > error prone. Define a "device" guard() definition to allow for those to "Define a definition" sounds a bit awkward, perhaps "Add a .."? > be cleaned up and prevent new ones from appearing. > > Link: http://lore.kernel.org/r/657897453dda8_269bd29492@dwillia2-mobl3.amr.corp.intel.com.notmuch > Link: http://lore.kernel.org/r/6577b0c2a02df_a04c5294bb@dwillia2-xfh.jf.intel.com.notmuch > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Other than that, looks good, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > --- > Hi Greg, > > I wonder if you might include this change in v6.7-rc to ease some patch > sets alternately going through my tree and Andrew's tree. Those > discussions are linked above. Alternately I can can just take it through > my tree with your ack and the other use case can circle back to it in > the v6.9 cycle. > > I considered also defining a __free() helper similar to __free(mutex), > but I think "__free(device)" would be a surprising name for something > that drops a lock. Also, I like the syntax of guard(device) over > something like guard(device_lock) since a 'struct device *' is the > argument, not a lock type, but I'm open to your or Peter's thoughts on > the naming. > > include/linux/device.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/device.h b/include/linux/device.h > index d7a72a8749ea..6c83294395ac 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -1007,6 +1007,8 @@ static inline void device_unlock(struct device *dev) > mutex_unlock(&dev->mutex); > } > > +DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T)) > + > static inline void device_lock_assert(struct device *dev) > { > lockdep_assert_held(&dev->mutex); >
On Wed, Dec 13, 2023 at 03:02:35PM -0800, Dan Williams wrote: > At present there are ~200 usages of device_lock() in the kernel. Some of > those usages lead to "goto unlock;" patterns which have proven to be > error prone. Define a "device" guard() definition to allow for those to > be cleaned up and prevent new ones from appearing. > > Link: http://lore.kernel.org/r/657897453dda8_269bd29492@dwillia2-mobl3.amr.corp.intel.com.notmuch > Link: http://lore.kernel.org/r/6577b0c2a02df_a04c5294bb@dwillia2-xfh.jf.intel.com.notmuch > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > Hi Greg, > > I wonder if you might include this change in v6.7-rc to ease some patch > sets alternately going through my tree and Andrew's tree. Those > discussions are linked above. Alternately I can can just take it through > my tree with your ack and the other use case can circle back to it in > the v6.9 cycle. Sure, I'll queue it up now for 6.7-final, makes sense to have it now for others to build off of, and for me to fix up some places in the driver core to use it as well. > I considered also defining a __free() helper similar to __free(mutex), > but I think "__free(device)" would be a surprising name for something > that drops a lock. Also, I like the syntax of guard(device) over > something like guard(device_lock) since a 'struct device *' is the > argument, not a lock type, but I'm open to your or Peter's thoughts on > the naming. guard(device); makes sense to me, as that's what you are doing here, so I'm good with it. thanks, greg k-h
On 12/13/23 16:02, Dan Williams wrote: > At present there are ~200 usages of device_lock() in the kernel. Some of > those usages lead to "goto unlock;" patterns which have proven to be > error prone. Define a "device" guard() definition to allow for those to > be cleaned up and prevent new ones from appearing. > > Link: http://lore.kernel.org/r/657897453dda8_269bd29492@dwillia2-mobl3.amr.corp.intel.com.notmuch > Link: http://lore.kernel.org/r/6577b0c2a02df_a04c5294bb@dwillia2-xfh.jf.intel.com.notmuch > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > Hi Greg, > > I wonder if you might include this change in v6.7-rc to ease some patch > sets alternately going through my tree and Andrew's tree. Those > discussions are linked above. Alternately I can can just take it through > my tree with your ack and the other use case can circle back to it in > the v6.9 cycle. > > I considered also defining a __free() helper similar to __free(mutex), > but I think "__free(device)" would be a surprising name for something > that drops a lock. Also, I like the syntax of guard(device) over > something like guard(device_lock) since a 'struct device *' is the > argument, not a lock type, but I'm open to your or Peter's thoughts on > the naming. > > include/linux/device.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/device.h b/include/linux/device.h > index d7a72a8749ea..6c83294395ac 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -1007,6 +1007,8 @@ static inline void device_unlock(struct device *dev) > mutex_unlock(&dev->mutex); > } > > +DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T)) > + > static inline void device_lock_assert(struct device *dev) > { > lockdep_assert_held(&dev->mutex); > >
diff --git a/include/linux/device.h b/include/linux/device.h index d7a72a8749ea..6c83294395ac 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -1007,6 +1007,8 @@ static inline void device_unlock(struct device *dev) mutex_unlock(&dev->mutex); } +DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T)) + static inline void device_lock_assert(struct device *dev) { lockdep_assert_held(&dev->mutex);
At present there are ~200 usages of device_lock() in the kernel. Some of those usages lead to "goto unlock;" patterns which have proven to be error prone. Define a "device" guard() definition to allow for those to be cleaned up and prevent new ones from appearing. Link: http://lore.kernel.org/r/657897453dda8_269bd29492@dwillia2-mobl3.amr.corp.intel.com.notmuch Link: http://lore.kernel.org/r/6577b0c2a02df_a04c5294bb@dwillia2-xfh.jf.intel.com.notmuch Cc: Vishal Verma <vishal.l.verma@intel.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- Hi Greg, I wonder if you might include this change in v6.7-rc to ease some patch sets alternately going through my tree and Andrew's tree. Those discussions are linked above. Alternately I can can just take it through my tree with your ack and the other use case can circle back to it in the v6.9 cycle. I considered also defining a __free() helper similar to __free(mutex), but I think "__free(device)" would be a surprising name for something that drops a lock. Also, I like the syntax of guard(device) over something like guard(device_lock) since a 'struct device *' is the argument, not a lock type, but I'm open to your or Peter's thoughts on the naming. include/linux/device.h | 2 ++ 1 file changed, 2 insertions(+)