On Mon, Feb 25, 2019 at 10:42:46AM -0800, Matthew Wilcox wrote:
> On Mon, Feb 25, 2019 at 07:57:33PM +0200, Ville Syrjälä wrote:
> > On Thu, Feb 21, 2019 at 10:41:23AM -0800, Matthew Wilcox wrote:
> > > @@ -49,8 +49,7 @@ struct drm_dp_aux_dev {
> > >
> > > #define DRM_AUX_MINORS 256
> > > #define AUX_MAX_OFFSET (1 << 20)
> > > -static DEFINE_IDR(aux_idr);
> > > -static DEFINE_MUTEX(aux_idr_mutex);
> > > +static DEFINE_XARRAY_ALLOC(aux_xa);
> > > static struct class *drm_dp_aux_dev_class;
> > > static int drm_dev_major = -1;
> [...]
> > > static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux
> > > *aux)
> > > {
> > > + static u32 next;
> >
> > Is there a particular reason for that being static?
>
> It needs to maintain its value between function calls so that we know
> where to start allocating from the next time we call xa_alloc_cyclic().
> It can either live here with a short name, or we could put it at file
> scope with a descriptive name (probably aux_xa_next). It's up to you
> which you think is better; it's your driver.
File scope would seem a bit more clear to me. I'm slightly worried
someone might do some cleanup here and drop the static without
thinking. Alterntively a short comment would probably suffice.
>
> The IDR embedded the 'next' value in the struct idr, but that was
> overhead paid by all users of the IDR rather than the very few that
> called idr_alloc_cyclic().
>
> > > + err = xa_alloc_cyclic(&aux_xa, &aux_dev->index, aux_dev,
> > > + XA_LIMIT(0, DRM_AUX_MINORS), &next, GFP_KERNEL);
> > > + if (err < 0) {
> > > kfree(aux_dev);
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
