Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Dec 2022 11:08:04 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Yunke Cao <yunkec@chromium.org>,
        Ming Lei <tom.leiming@gmail.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Max Staudt <mstaudt@google.com>, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org
Subject: Re: [PATCH v2] media: uvcvideo: Do not alloc dev->status
Message-ID: <Y5rj9GhQuyNbNnDw@pendragon.ideasonboard.com>
References: <20221214-uvc-status-alloc-v2-0-3f1cba6fc734@chromium.org>
 <Y5p1DVWXuYSzkRO4@google.com>
 <CANiDSCtQFR-CPNXu-hqM_11DogMTvdhK9xWy7cZyd7vwZAMBSQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CANiDSCtQFR-CPNXu-hqM_11DogMTvdhK9xWy7cZyd7vwZAMBSQ@mail.gmail.com>
Precedence: bulk

Hi Ricardo,

On Thu, Dec 15, 2022 at 08:59:14AM +0100, Ricardo Ribalda wrote:
> Hi Sergey
> 
> Thanks for looking into this
> 
> On Thu, 15 Dec 2022 at 02:15, Sergey Senozhatsky wrote:
> >
> > On (22/12/14 14:37), Ricardo Ribalda wrote:
> > [..]
> > > +struct uvc_status_streaming {
> > > +     u8      button;
> > > +} __packed;
> > > +
> > > +struct uvc_status_control {
> > > +     u8      bSelector;
> > > +     u8      bAttribute;
> > > +     u8      bValue[11];
> > > +} __packed;
> > > +
> > > +struct uvc_status {
> > > +     u8      bStatusType;
> > > +     u8      bOriginator;
> > > +     u8      bEvent;
> > > +     union {
> > > +             struct uvc_status_control control;
> > > +             struct uvc_status_streaming streaming;
> > > +     };
> > > +} __packed;
> > > +
> > >  struct uvc_device {
> > >       struct usb_device *udev;
> > >       struct usb_interface *intf;
> > > @@ -559,7 +579,7 @@ struct uvc_device {
> > >       /* Status Interrupt Endpoint */
> > >       struct usb_host_endpoint *int_ep;
> > >       struct urb *int_urb;
> > > -     u8 *status;
> > > +
> > >       struct input_dev *input;
> > >       char input_phys[64];
> > >
> > > @@ -572,6 +592,12 @@ struct uvc_device {
> > >       } async_ctrl;
> > >
> > >       struct uvc_entity *gpio_unit;
> > > +
> > > +     /*
> > > +      * Ensure that status is aligned, making it safe to use with
> > > +      * non-coherent DMA.
> > > +      */
> > > +     struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
> >
> >         ____cacheline_aligned ?
> >
> > I don't see anyone using ARCH_KMALLOC_MINALIGN except for slab.h
> 
> Seems like cacheline is not good enough:
> 
> https://github.com/torvalds/linux/commit/12c4efe3509b8018e76ea3ebda8227cb53bf5887
> https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/
> 
> and ARCH_KMALLOC_MINALIGN is what we have today and is working...
> 
> But yeah, the name for that define is not the nicest :)
> 
> I added Jonathan Cameron, on cc, as he had to deal with something
> similar for iio in case we are missing something

I'd like to get feedback on this from DMA and USB experts. Expanding the
CC list of the original patch would help (especially including the
linux-usb mailing list).

> ps: and I thought this was an easy change :P

-- 
Regards,

Laurent Pinchart