Re: [libgpiod] [PATCH 11/19] API: add support for SET_CONFIG

[Bartosz Golaszewski <brgl@xxxxxxxx>]

czw., 21 lis 2019 o 01:34 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
>
> On Wed, Nov 20, 2019 at 04:18:24PM +0100, Bartosz Golaszewski wrote:
> > śr., 20 lis 2019 o 15:36 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> > >
> > > On Wed, Nov 20, 2019 at 03:18:36PM +0100, Bartosz Golaszewski wrote:
> > > > śr., 20 lis 2019 o 15:13 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> > > > >
> > > > > On Wed, Nov 20, 2019 at 03:08:57PM +0100, Bartosz Golaszewski wrote:
> > > > > > śr., 20 lis 2019 o 14:59 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> > > > > > >
> > > > > > > On Wed, Nov 20, 2019 at 12:00:45PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > wt., 19 lis 2019 o 16:53 Kent Gibson <warthog618@xxxxxxxxx> napisał(a):
> > > > > > > > >
> > > > > > > > > On Mon, Nov 18, 2019 at 10:48:25PM +0800, Kent Gibson wrote:
> > > > > > > > > > On Mon, Nov 18, 2019 at 02:52:04PM +0100, Bartosz Golaszewski
> > > > > > > > > > > > +
> > > > > > > > > > > > +int gpiod_line_set_flags_bulk(struct gpiod_line_bulk *bulk, int flags)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +       struct gpiod_line *line;
> > > > > > > > > > > > +       int values[GPIOD_LINE_BULK_MAX_LINES];
> > > > > > > > > > > > +       unsigned int i;
> > > > > > > > > > > > +       int direction;
> > > > > > > > > > > > +
> > > > > > > > > > > > +       line = gpiod_line_bulk_get_line(bulk, 0);
> > > > > > > > > > > > +       if (line->as_is) {
> > > > > > > > > > >
> > > > > > > > > > > Can you explain the purpose of this as_is field? I'm not sure this is
> > > > > > > > > > > really needed.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > It is there for gpiod_set_flags, which has to populate the direction
> > > > > > > > > > flags in the SET_CONFIG ioctl. The existing line->direction is
> > > > > > > > > > either input or output.  It is drawn from GPIOLINE_FLAG_IS_OUT, so
> > > > > > > > > > as-is is gets mapped to input.
> > > > > > > > > > I didn't want to change the existing line->direction, and adding the
> > > > > > > > > > as-is seemed clearer than adding another flavour of direction that
> > > > > > > > > > contained all three.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hmmm, I think I see what you were getting at - the line->direction is the
> > > > > > > > > direction from the kernel, so it doesn't hurt to use that value to set the
> > > > > > > > > corresponding request flags - even if the original request was as-is??
> > > > > > > > >
> > > > > > > > > If that is the case then the line->as_is can be dropped throughout.
> > > > > > > > >
> > > > > > > > > Kent.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes, this is what I was thinking. Just need to make sure the value
> > > > > > > > from the kernel is up-to-date.
> > > > > > > >
> > > > > > >
> > > > > > > So fail if needs_update?
> > > > > > >
> > > > > > > Kent.
> > > > > >
> > > > > > I'd say: do an implicit update before setting config.
> > > > > >
> > > > >
> > > > > So gpiod_line_update if needs_update, and fail if that fails?
> > > > >
> > > > > Kent.
> > > >
> > > > Without the if - needs_update is only set if an implicit update fails
> > > > in line_maybe_update(). But in this case we need to be sure, so do it
> > > > unconditionally.
> > > >
> > >
> > > Given that line_maybe_update is called at the end of request creation, and
> > > whenever set_config is called, how can line->direction be inconsistent
> > > with the kernel state - as long as needs_update is false?
> > >
> >
> > I don't think we should call line_maybe_update() on set_config() - in
> > this case we should call gpiod_line_update() and fail in set_config()
> > if it fails.
> >
> > I hope that's clearer.
> >
>
> Not really.  I was already shaky on the needs_update and I'm getting more
> confused about the overall needs_update handling policy by the minute.
>

Yeah it's not optimal. If you have better ideas on how to handle the
fact that the kernel can't really notify us about the changes in
line's flags introduced by other processes - I'll be more than glad to
give them a try. At some point I was thinking about another ioctl()
that - for a requested line - would return a file descriptor which
would emit events when a line changes - for instance, it's requested
by someone else or its direction is changed etc.

I then thought that nobody is requesting this yet and so it may be overkill.

Bart

> Perhaps it will be more productive to go to the code.
> I'll send out what I have as v2 and we can go from there.
>
> Cheers,
> Kent.