ichsmb(4) and msleep()

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

ichsmb(4) and msleep()

yuripv
I have a "timed sleep before timers are working" panic in ichsmb_readb()
calling ichsmb_wait() which uses msleep().  That is trying to
jedec_dimm(4) module so it's trying to attach pretty early in boot.
What would be the correct replacement for msleep() here?

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

Hans Petter Selasky-6
On 2019-08-28 11:07, Yuri Pankov wrote:
> I have a "timed sleep before timers are working" panic in ichsmb_readb()
> calling ichsmb_wait() which uses msleep().  That is trying to
> jedec_dimm(4) module so it's trying to attach pretty early in boot.
> What would be the correct replacement for msleep() here?
>

If you only need a sleep-delay, pause() is the right one. It handles
cold-boot.

--HPS

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

yuripv
Hans Petter Selasky wrote:
> On 2019-08-28 11:07, Yuri Pankov wrote:
>> I have a "timed sleep before timers are working" panic in ichsmb_readb()
>> calling ichsmb_wait() which uses msleep().  That is trying to
>> jedec_dimm(4) module so it's trying to attach pretty early in boot.
>> What would be the correct replacement for msleep() here?
>>
>
> If you only need a sleep-delay, pause() is the right one. It handles
> cold-boot.

I guess that won't work here as we need to be waked up by interrupt
handler on command completion, and pause() seems to sleep
unconditionally for the given time in 'cold' case (if I'm reading the
code correctly).
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

Hans Petter Selasky-6
On 2019-08-28 11:44, Yuri Pankov wrote:

> Hans Petter Selasky wrote:
>> On 2019-08-28 11:07, Yuri Pankov wrote:
>>> I have a "timed sleep before timers are working" panic in ichsmb_readb()
>>> calling ichsmb_wait() which uses msleep().  That is trying to
>>> jedec_dimm(4) module so it's trying to attach pretty early in boot.
>>> What would be the correct replacement for msleep() here?
>>>
>>
>> If you only need a sleep-delay, pause() is the right one. It handles
>> cold-boot.
>
> I guess that won't work here as we need to be waked up by interrupt
> handler on command completion, and pause() seems to sleep
> unconditionally for the given time in 'cold' case (if I'm reading the
> code correctly).

If you are too early inside a SYSINIT() path, then you cannot use
sleeping. You will have to use polling in a loop with a fixed DELAY() to
know the timeout.

--HPS

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

yuripv
Hans Petter Selasky wrote:

> On 2019-08-28 11:44, Yuri Pankov wrote:
>> Hans Petter Selasky wrote:
>>> On 2019-08-28 11:07, Yuri Pankov wrote:
>>>> I have a "timed sleep before timers are working" panic in ichsmb_readb()
>>>> calling ichsmb_wait() which uses msleep().  That is trying to
>>>> jedec_dimm(4) module so it's trying to attach pretty early in boot.
>>>> What would be the correct replacement for msleep() here?
>>>>
>>>
>>> If you only need a sleep-delay, pause() is the right one. It handles
>>> cold-boot.
>>
>> I guess that won't work here as we need to be waked up by interrupt
>> handler on command completion, and pause() seems to sleep
>> unconditionally for the given time in 'cold' case (if I'm reading the
>> code correctly).
>
> If you are too early inside a SYSINIT() path, then you cannot use
> sleeping. You will have to use polling in a loop with a fixed DELAY() to
> know the timeout.
Thanks for the help.

Something like the following (it seems to work)?



_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"

ichsmb.diff.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

yuripv
Yuri Pankov wrote:

> Hans Petter Selasky wrote:
>> On 2019-08-28 11:44, Yuri Pankov wrote:
>>> Hans Petter Selasky wrote:
>>>> On 2019-08-28 11:07, Yuri Pankov wrote:
>>>>> I have a "timed sleep before timers are working" panic in ichsmb_readb()
>>>>> calling ichsmb_wait() which uses msleep().  That is trying to
>>>>> jedec_dimm(4) module so it's trying to attach pretty early in boot.
>>>>> What would be the correct replacement for msleep() here?
>>>>>
>>>>
>>>> If you only need a sleep-delay, pause() is the right one. It handles
>>>> cold-boot.
>>>
>>> I guess that won't work here as we need to be waked up by interrupt
>>> handler on command completion, and pause() seems to sleep
>>> unconditionally for the given time in 'cold' case (if I'm reading the
>>> code correctly).
>>
>> If you are too early inside a SYSINIT() path, then you cannot use
>> sleeping. You will have to use polling in a loop with a fixed DELAY() to
>> know the timeout.
>
> Thanks for the help.
>
> Something like the following (it seems to work)?

Here's a review with the nit you mentioned fixed, thanks!

https://reviews.freebsd.org/D21452
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

Warner Losh
On Wed, Aug 28, 2019, 7:01 AM Yuri Pankov <[hidden email]> wrote:

> Yuri Pankov wrote:
> > Hans Petter Selasky wrote:
> >> On 2019-08-28 11:44, Yuri Pankov wrote:
> >>> Hans Petter Selasky wrote:
> >>>> On 2019-08-28 11:07, Yuri Pankov wrote:
> >>>>> I have a "timed sleep before timers are working" panic in
> ichsmb_readb()
> >>>>> calling ichsmb_wait() which uses msleep().  That is trying to
> >>>>> jedec_dimm(4) module so it's trying to attach pretty early in boot.
> >>>>> What would be the correct replacement for msleep() here?
> >>>>>
> >>>>
> >>>> If you only need a sleep-delay, pause() is the right one. It handles
> >>>> cold-boot.
> >>>
> >>> I guess that won't work here as we need to be waked up by interrupt
> >>> handler on command completion, and pause() seems to sleep
> >>> unconditionally for the given time in 'cold' case (if I'm reading the
> >>> code correctly).
> >>
> >> If you are too early inside a SYSINIT() path, then you cannot use
> >> sleeping. You will have to use polling in a loop with a fixed DELAY()
> to
> >> know the timeout.
> >
> > Thanks for the help.
> >
> > Something like the following (it seems to work)?
>
> Here's a review with the nit you mentioned fixed, thanks!
>
> https://reviews.freebsd.org/D21452



What's the advantages of doing this instead of deferring attach until the
interrupts are running?

Warner

>
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

yuripv
Warner Losh wrote:

> On Wed, Aug 28, 2019, 7:01 AM Yuri Pankov <[hidden email]> wrote:
>
>> Yuri Pankov wrote:
>>> Hans Petter Selasky wrote:
>>>> On 2019-08-28 11:44, Yuri Pankov wrote:
>>>>> Hans Petter Selasky wrote:
>>>>>> On 2019-08-28 11:07, Yuri Pankov wrote:
>>>>>>> I have a "timed sleep before timers are working" panic in
>> ichsmb_readb()
>>>>>>> calling ichsmb_wait() which uses msleep().  That is trying to
>>>>>>> jedec_dimm(4) module so it's trying to attach pretty early in boot.
>>>>>>> What would be the correct replacement for msleep() here?
>>>>>>>
>>>>>>
>>>>>> If you only need a sleep-delay, pause() is the right one. It handles
>>>>>> cold-boot.
>>>>>
>>>>> I guess that won't work here as we need to be waked up by interrupt
>>>>> handler on command completion, and pause() seems to sleep
>>>>> unconditionally for the given time in 'cold' case (if I'm reading the
>>>>> code correctly).
>>>>
>>>> If you are too early inside a SYSINIT() path, then you cannot use
>>>> sleeping. You will have to use polling in a loop with a fixed DELAY()
>> to
>>>> know the timeout.
>>>
>>> Thanks for the help.
>>>
>>> Something like the following (it seems to work)?
>>
>> Here's a review with the nit you mentioned fixed, thanks!
>>
>> https://reviews.freebsd.org/D21452
>
>
>
> What's the advantages of doing this instead of deferring attach until the
> interrupts are running?

None that I can think of, just going with what was suggested and seeing
other drivers doing the same.  Could you please name a driver that
defers attach until !cold?
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

Ian Lepore-3
In reply to this post by yuripv
On Wed, 2019-08-28 at 14:41 +0300, Yuri Pankov wrote:

> Hans Petter Selasky wrote:
> > On 2019-08-28 11:44, Yuri Pankov wrote:
> > > Hans Petter Selasky wrote:
> > > > On 2019-08-28 11:07, Yuri Pankov wrote:
> > > > > I have a "timed sleep before timers are working" panic in
> > > > > ichsmb_readb()
> > > > > calling ichsmb_wait() which uses msleep().  That is trying to
> > > > > jedec_dimm(4) module so it's trying to attach pretty early in
> > > > > boot.
> > > > > What would be the correct replacement for msleep() here?
> > > > >
> > > >
> > > > If you only need a sleep-delay, pause() is the right one. It
> > > > handles
> > > > cold-boot.
> > >
> > > I guess that won't work here as we need to be waked up by
> > > interrupt
> > > handler on command completion, and pause() seems to sleep
> > > unconditionally for the given time in 'cold' case (if I'm reading
> > > the
> > > code correctly).
> >
> > If you are too early inside a SYSINIT() path, then you cannot use
> > sleeping. You will have to use polling in a loop with a fixed
> > DELAY() to
> > know the timeout.
>
> Thanks for the help.
>
> Something like the following (it seems to work)?
>

Do you actually need to communicate with these i2c slave devices as
part of making the system usable early in boot?  That sometimes happens
on embedded systems where you need to communicate with a power-
management system via i2c to bring up other devices.  In that case you
need to do polling until interrupts are available.

If not, then this is probably fallout from a historic freebsd glitch
where almost all i2c controller drivers attach the iicbus child before
interrupts are available, and then rely on all the slave device drivers
to not do any IO until interrupts are available.  IMO, that's insane...
a driver shouldn't attach children until it's in a state where it's
able to service the IO requests from them.

The simple fix is usually to change the last few lines of the
controller's attach() code.  Usually it's something like

  return bus_generic_attach(dev);

and you can just change it to

  config_intrhook_oneshot((ich_func_t)bus_generic_attach, dev);
  return (0);

-- Ian

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

Conrad Meyer-2
In reply to this post by yuripv
On Wed, Aug 28, 2019 at 6:35 AM Yuri Pankov <[hidden email]> wrote:
>
> Warner Losh wrote:
> > What's the advantages of doing this instead of deferring attach until the
> > interrupts are running?
>
> None that I can think of, just going with what was suggested and seeing
> other drivers doing the same.  Could you please name a driver that
> defers attach until !cold?

I think pretty much all drivers attach when interrupts are enabled
(not the same as !cold)?  At least x86 enables interrupts on BSP at
SI_SUB_INTR, and DRIVER_MODULE drivers *load* at SI_SUB_DRIVERS, and
the INTR one is ordered before the other.  My skim read is that
drivers do not actually attach until SI_SUB_CONFIGURE.

I think the panic / test in sleepq_set_timeout_sbt is maybe overly
strong?  !cold indicates the entire autoconfigure process has
concluded.  But interrupts are available long before that.  Seems like
hardclock is started at ~SI_SUB_CLOCKS?  Which is admittedly after
DRIVERS, but still long before !cold.  I'm not sure what set of
interrupt/timer functionality is needed for sleepq, but likely that
condition can be relaxed.

If it cannot be relaxed enough for your driver, you could expand your
DRIVER_MODULE() into the expanded macro, replacing SI_SUB_DRIVERS with
a later stage.

Best,
Conrad
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

yuripv
Conrad Meyer wrote:

> On Wed, Aug 28, 2019 at 6:35 AM Yuri Pankov <[hidden email]> wrote:
>>
>> Warner Losh wrote:
>>> What's the advantages of doing this instead of deferring attach until the
>>> interrupts are running?
>>
>> None that I can think of, just going with what was suggested and seeing
>> other drivers doing the same.  Could you please name a driver that
>> defers attach until !cold?
>
> I think pretty much all drivers attach when interrupts are enabled
> (not the same as !cold)?  At least x86 enables interrupts on BSP at
> SI_SUB_INTR, and DRIVER_MODULE drivers *load* at SI_SUB_DRIVERS, and
> the INTR one is ordered before the other.  My skim read is that
> drivers do not actually attach until SI_SUB_CONFIGURE.
>
> I think the panic / test in sleepq_set_timeout_sbt is maybe overly
> strong?  !cold indicates the entire autoconfigure process has
> concluded.  But interrupts are available long before that.  Seems like
> hardclock is started at ~SI_SUB_CLOCKS?  Which is admittedly after
> DRIVERS, but still long before !cold.  I'm not sure what set of
> interrupt/timer functionality is needed for sleepq, but likely that
> condition can be relaxed.
>
> If it cannot be relaxed enough for your driver, you could expand your
> DRIVER_MODULE() into the expanded macro, replacing SI_SUB_DRIVERS with
> a later stage.

Yes, thank you.  I guess it's already sorted out with Ian's help, please
see the review:

https://reviews.freebsd.org/D21452
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

Ian Lepore-3
In reply to this post by Conrad Meyer-2
On Wed, 2019-08-28 at 20:27 -0700, Conrad Meyer wrote:

> On Wed, Aug 28, 2019 at 6:35 AM Yuri Pankov <[hidden email]> wrote:
> >
> > Warner Losh wrote:
> > > What's the advantages of doing this instead of deferring attach until the
> > > interrupts are running?
> >
> > None that I can think of, just going with what was suggested and seeing
> > other drivers doing the same.  Could you please name a driver that
> > defers attach until !cold?
>
> I think pretty much all drivers attach when interrupts are enabled
> (not the same as !cold)?  At least x86 enables interrupts on BSP at
> SI_SUB_INTR, and DRIVER_MODULE drivers *load* at SI_SUB_DRIVERS, and
> the INTR one is ordered before the other.  My skim read is that
> drivers do not actually attach until SI_SUB_CONFIGURE.
>
> I think the panic / test in sleepq_set_timeout_sbt is maybe overly
> strong?  !cold indicates the entire autoconfigure process has
> concluded.  But interrupts are available long before that.  Seems like
> hardclock is started at ~SI_SUB_CLOCKS?  Which is admittedly after
> DRIVERS, but still long before !cold.  I'm not sure what set of
> interrupt/timer functionality is needed for sleepq, but likely that
> condition can be relaxed.
>
> If it cannot be relaxed enough for your driver, you could expand your
> DRIVER_MODULE() into the expanded macro, replacing SI_SUB_DRIVERS with
> a later stage.
>
>

Afaik, only x86 enables interrupts before SI_SUB_CONFIGURE.  Maybe
sparc64 does too.  Other arches do it as the last thing in
SI_SUB_CONFIGURE, usually in a function named configure_final() in
<arch>/autoconf.c.

-- Ian


_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: ichsmb(4) and msleep()

Warner Losh
On Thu, Aug 29, 2019 at 8:29 AM Ian Lepore <[hidden email]> wrote:

> On Wed, 2019-08-28 at 20:27 -0700, Conrad Meyer wrote:
> > On Wed, Aug 28, 2019 at 6:35 AM Yuri Pankov <[hidden email]> wrote:
> > >
> > > Warner Losh wrote:
> > > > What's the advantages of doing this instead of deferring attach
> until the
> > > > interrupts are running?
> > >
> > > None that I can think of, just going with what was suggested and seeing
> > > other drivers doing the same.  Could you please name a driver that
> > > defers attach until !cold?
> >
> > I think pretty much all drivers attach when interrupts are enabled
> > (not the same as !cold)?  At least x86 enables interrupts on BSP at
> > SI_SUB_INTR, and DRIVER_MODULE drivers *load* at SI_SUB_DRIVERS, and
> > the INTR one is ordered before the other.  My skim read is that
> > drivers do not actually attach until SI_SUB_CONFIGURE.
> >
> > I think the panic / test in sleepq_set_timeout_sbt is maybe overly
> > strong?  !cold indicates the entire autoconfigure process has
> > concluded.  But interrupts are available long before that.  Seems like
> > hardclock is started at ~SI_SUB_CLOCKS?  Which is admittedly after
> > DRIVERS, but still long before !cold.  I'm not sure what set of
> > interrupt/timer functionality is needed for sleepq, but likely that
> > condition can be relaxed.
> >
> > If it cannot be relaxed enough for your driver, you could expand your
> > DRIVER_MODULE() into the expanded macro, replacing SI_SUB_DRIVERS with
> > a later stage.
> >
> >
>
> Afaik, only x86 enables interrupts before SI_SUB_CONFIGURE.  Maybe
> sparc64 does too.  Other arches do it as the last thing in
> SI_SUB_CONFIGURE, usually in a function named configure_final() in
> <arch>/autoconf.c.
>

Yes. In fact, I think this idiom is so wide spread, we'd be better off
doing 'return (bus_delayed_attach_children(dev));' at the end to signal
this and gather together all current uses in the tree under that.
Generally, if you need interrupts enabled and scheduling running to
configure your children, you should do that anyway. It's a convenient late
point, and it's better to do all the late things at the same time with the
same idiom than to try to hyper-optimize when they get done. We've cleaned
up a number of messes where driver writers invent their own thing due to
ignorance of the proper thing (or because there wasn't a recognition that
it was similar to what lots of other drivers did).

Warner
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"