Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl
       [not found]               ` <CAL1e-=g8X___59zLPKLRjFNAP9bs3rVWhc8+OhMuF3TriBiynw@mail.gmail.com>
@ 2020-01-16 12:00                 ` Arnd Bergmann
  2020-01-17 20:50                   ` Aleksandar Markovic
  0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2020-01-16 12:00 UTC (permalink / raw)
  To: Aleksandar Markovic
  Cc: Laurent Vivier, Filip Bozuta, Peter Maydell,
	Daniel P. Berrangé,
	Richard Henderson, qemu-devel, Max Filippov, amarkovic, philmd,
	Alexandre Belloni, linux-rtc

On Thu, Jan 16, 2020 at 12:27 PM Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
> On Thursday, January 16, 2020, Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote:
>> On Wednesday, January 15, 2020, Laurent Vivier <laurent@vivier.eu> wrote:
>>> Le 15/01/2020 à 20:17, Filip Bozuta a écrit :
>>> > On 15.1.20. 17:37, Arnd Bergmann wrote:
>>> >> On Wed, Jan 15, 2020 at 5:32 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>> >>> Le 15/01/2020 à 17:18, Arnd Bergmann a écrit :
>>> >>>> On Wed, Jan 15, 2020 at 4:53 PM Filip Bozuta
>>> >>>> <Filip.Bozuta@rt-rk.com> wrote:
>>> >>>>> This patch implements functionality of following ioctl:
>>> >>>>>
>>> >>>>> SNDRV_TIMER_IOCTL_TREAD - Setting enhanced time read
>>> >>>>>
>>> >>>>>      Sets enhanced time read which is used for reading time with
>>> >>>>> timestamps
>>> >>>>>      and events. The third ioctl's argument is a pointer to an
>>> >>>>> 'int'. Enhanced
>>> >>>>>      reading is set if the third argument is different than 0,
>>> >>>>> otherwise normal
>>> >>>>>      time reading is set.
>>> >>>>>
>>> >>>>> Implementation notes:
>>> >>>>>
>>> >>>>>      Because the implemented ioctl has 'int' as its third argument,
>>> >>>>> the
>>> >>>>>      implementation was straightforward.
>>> >>>>>
>>> >>>>> Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
>>> >>>> I think this one is wrong when you go between 32-bit and 64-bit
>>> >>> kernel uses an "int" and "int" is always 32bit.
>>> >>> The problem is most likely with timespec I think.
>>> >>>
>>> >>>> targets, and it gets worse with the kernel patches that just got
>>> >>>> merged for linux-5.5, which extends the behavior to deal with
>>> >>>> 64-bit time_t on 32-bit architectures.
>>> >>>>
>>> >>>> Please have a look at
>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?h=80fe7430c70859
>>> >>>>
>>> >>> Yes, we already had the same kind of problem with SIOCGSTAMP and
>>> >>> SIOCGSTAMPNS.
>>> >>>
>>> >>> Do the kernel patches add new ioctl numbers to differentiate 32bit and
>>> >>> 64bit time_t?
>>> >> Yes, though SNDRV_TIMER_IOCTL_TREAD is worse: the ioctl argument
>>> >> is a pure 'int' that decides what format you get when you 'read' from the
>>> >> same file descriptor.
>>> >>
>>> >> For emulating 64-bit on 32-bit kernels, you have to use the new
>>> >> SNDRV_TIMER_IOCTL_TREAD64, and for emulating 32-bit on
>>> >> 64-bit kernels, you probably have to return -ENOTTY to
>>> >> SNDRV_TIMER_IOCTL_TREAD_OLD unless you also want to
>>> >> emulate the read() behavior.
>>> >> When a 32-bit process calls SNDRV_TIMER_IOCTL_TREAD64,
>>> >> you can translate that into the 64-bit
>>> >> SNDRV_TIMER_IOCTL_TREAD_OLD.
>>> >
>>> > Thank you for bringing this up to my attention. Unfortunately i have
>>> > some duties of academic nature in next month so i won't have much time
>>> > fix this bug. I will try to fix this as soon as possible.
>>>
>>> Could you at least to try to have a mergeable series before you have to
>>> stop to work on this?
>>>
>>> You can only manage the case before the change reported by Arnd (I will
>>> manage the new case myself later).

>>
>> Sorry for interjecting myself into this discussion, but I just want to let you know about some related practicalities.
>>
>> Filip is a student that is from time to time (usually between two exam seasons) an intern in our company, working 4 hours each work day. He spent his internship in different teams in prevous years, and, from around mid-October 2019, was appointed to QEMU team. After some introductory tasks, he was assigned his main task: linux-user support for RTCs and ALSA timers. This series is the result of his work, and, to my great pleasure, is virtually entirely his independant work. I am positive he can complete the series by himself, even in the light of additional complexities mentioned in this thread.
>>
>> However, his exam season just started (Jan. 15th), and lasts till Feb. 15th. Our policy, in general, is not to burden the students during exam seasons, and that is why we can't expect prompt updates from him for the time being.
>>
>> In view of this, Laurent, please take Filip's status into consideration. As far as mergeability is concerned, my impression is that patches 1-6 and 13 are ready for merging, while patches 7-12 would require some additional (netlink-support-like) work, that would unfortunately be possible only after Feb. 15th.
>>

> Laurent, hi again.
>
> I am not completely familiar with all details of Filip's work, since, as I said, he had
> large degree of independance (which was intentional, and is a desired and good
> thing IMHO), but taking a closer look, a question starting lingering: Do we need
> special handling od read() even for RTC devices - not only ALSA timer devices?

Adding Alexandre Belloni and the RTC list to Cc for more expertise. Alexandre,
this question is about how qemu-user should emulate the rtc ioctl interface when
running a user binary for a foreign architecture. The ioctl emulation seems fine
to me, but read() from /dev/rtc is probably not.

As I understand it, reading from /dev/rtc is one of the more obscure features.
This would return either 32 bits or 64 bits of structured data from the kernel,
depending on how much data was requested and whether the kernel runs
as 64 bit. A 32-bit process running on a 64-bit kernel will get the correct
result when it asks for 4 bytes, but probably not when it asks for 8 bytes.
(we could fix this with an explict check for in_compat_syscall() in the kernel
function).

A process running on qemu with the opposite endianess will always get the
wrong result (unless the kernel returns 0), and emulating 64-bit task on
a 32-bit kernel will result in only four bytes to be read, which also likely
results in incorrect behavior.

       Arnd

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl
  2020-01-16 12:00                 ` [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl Arnd Bergmann
@ 2020-01-17 20:50                   ` Aleksandar Markovic
  2020-01-17 21:45                     ` Arnd Bergmann
  2020-01-17 21:54                     ` Alexandre Belloni
  0 siblings, 2 replies; 4+ messages in thread
From: Aleksandar Markovic @ 2020-01-17 20:50 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Laurent Vivier, Filip Bozuta, Peter Maydell,
	Daniel P. Berrangé,
	Richard Henderson, qemu-devel, Max Filippov, amarkovic, philmd,
	Alexandre Belloni, linux-rtc

On Thu, Jan 16, 2020 at 1:00 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jan 16, 2020 at 12:27 PM Aleksandar Markovic
> <aleksandar.m.mail@gmail.com> wrote:
> > On Thursday, January 16, 2020, Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote:
> >> On Wednesday, January 15, 2020, Laurent Vivier <laurent@vivier.eu> wrote:
> >>> Le 15/01/2020 à 20:17, Filip Bozuta a écrit :
> >>> > On 15.1.20. 17:37, Arnd Bergmann wrote:
> >>> >> On Wed, Jan 15, 2020 at 5:32 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>> >>> Le 15/01/2020 à 17:18, Arnd Bergmann a écrit :
> >>> >>>> On Wed, Jan 15, 2020 at 4:53 PM Filip Bozuta
> >>> >>>> <Filip.Bozuta@rt-rk.com> wrote:
> >>> >>>>> This patch implements functionality of following ioctl:
> >>> >>>>>
> >>> >>>>> SNDRV_TIMER_IOCTL_TREAD - Setting enhanced time read
> >>> >>>>>
> >>> >>>>>      Sets enhanced time read which is used for reading time with
> >>> >>>>> timestamps
> >>> >>>>>      and events. The third ioctl's argument is a pointer to an
> >>> >>>>> 'int'. Enhanced
> >>> >>>>>      reading is set if the third argument is different than 0,
> >>> >>>>> otherwise normal
> >>> >>>>>      time reading is set.
> >>> >>>>>
> >>> >>>>> Implementation notes:
> >>> >>>>>
> >>> >>>>>      Because the implemented ioctl has 'int' as its third argument,
> >>> >>>>> the
> >>> >>>>>      implementation was straightforward.
> >>> >>>>>
> >>> >>>>> Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
> >>> >>>> I think this one is wrong when you go between 32-bit and 64-bit
> >>> >>> kernel uses an "int" and "int" is always 32bit.
> >>> >>> The problem is most likely with timespec I think.
> >>> >>>
> >>> >>>> targets, and it gets worse with the kernel patches that just got
> >>> >>>> merged for linux-5.5, which extends the behavior to deal with
> >>> >>>> 64-bit time_t on 32-bit architectures.
> >>> >>>>
> >>> >>>> Please have a look at
> >>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?h=80fe7430c70859
> >>> >>>>
> >>> >>> Yes, we already had the same kind of problem with SIOCGSTAMP and
> >>> >>> SIOCGSTAMPNS.
> >>> >>>
> >>> >>> Do the kernel patches add new ioctl numbers to differentiate 32bit and
> >>> >>> 64bit time_t?
> >>> >> Yes, though SNDRV_TIMER_IOCTL_TREAD is worse: the ioctl argument
> >>> >> is a pure 'int' that decides what format you get when you 'read' from the
> >>> >> same file descriptor.
> >>> >>
> >>> >> For emulating 64-bit on 32-bit kernels, you have to use the new
> >>> >> SNDRV_TIMER_IOCTL_TREAD64, and for emulating 32-bit on
> >>> >> 64-bit kernels, you probably have to return -ENOTTY to
> >>> >> SNDRV_TIMER_IOCTL_TREAD_OLD unless you also want to
> >>> >> emulate the read() behavior.
> >>> >> When a 32-bit process calls SNDRV_TIMER_IOCTL_TREAD64,
> >>> >> you can translate that into the 64-bit
> >>> >> SNDRV_TIMER_IOCTL_TREAD_OLD.
> >>> >
> >>> > Thank you for bringing this up to my attention. Unfortunately i have
> >>> > some duties of academic nature in next month so i won't have much time
> >>> > fix this bug. I will try to fix this as soon as possible.
> >>>
> >>> Could you at least to try to have a mergeable series before you have to
> >>> stop to work on this?
> >>>
> >>> You can only manage the case before the change reported by Arnd (I will
> >>> manage the new case myself later).
>
> >>
> >> Sorry for interjecting myself into this discussion, but I just want to let you know about some related practicalities.
> >>
> >> Filip is a student that is from time to time (usually between two exam seasons) an intern in our company, working 4 hours each work day. He spent his internship in different teams in prevous years, and, from around mid-October 2019, was appointed to QEMU team. After some introductory tasks, he was assigned his main task: linux-user support for RTCs and ALSA timers. This series is the result of his work, and, to my great pleasure, is virtually entirely his independant work. I am positive he can complete the series by himself, even in the light of additional complexities mentioned in this thread.
> >>
> >> However, his exam season just started (Jan. 15th), and lasts till Feb. 15th. Our policy, in general, is not to burden the students during exam seasons, and that is why we can't expect prompt updates from him for the time being.
> >>
> >> In view of this, Laurent, please take Filip's status into consideration. As far as mergeability is concerned, my impression is that patches 1-6 and 13 are ready for merging, while patches 7-12 would require some additional (netlink-support-like) work, that would unfortunately be possible only after Feb. 15th.
> >>
>
> > Laurent, hi again.
> >
> > I am not completely familiar with all details of Filip's work, since, as I said, he had
> > large degree of independance (which was intentional, and is a desired and good
> > thing IMHO), but taking a closer look, a question starting lingering: Do we need
> > special handling od read() even for RTC devices - not only ALSA timer devices?
>
> Adding Alexandre Belloni and the RTC list to Cc for more expertise. Alexandre,
> this question is about how qemu-user should emulate the rtc ioctl interface when
> running a user binary for a foreign architecture. The ioctl emulation seems fine
> to me, but read() from /dev/rtc is probably not.
>
> As I understand it, reading from /dev/rtc is one of the more obscure features.
> This would return either 32 bits or 64 bits of structured data from the kernel,
> depending on how much data was requested and whether the kernel runs
> as 64 bit. A 32-bit process running on a 64-bit kernel will get the correct
> result when it asks for 4 bytes, but probably not when it asks for 8 bytes.
> (we could fix this with an explict check for in_compat_syscall() in the kernel
> function).
>
> A process running on qemu with the opposite endianess will always get the
> wrong result (unless the kernel returns 0), and emulating 64-bit task on
> a 32-bit kernel will result in only four bytes to be read, which also likely
> results in incorrect behavior.
>


Alexandre (and Arnd too, or any other person knowledgeable in the area),

I just need to clarify a couple of details with you, please.

Firstly, here is what man page rtc(4) says:

"The /dev/rtc (or /dev/rtc0, /dev/rtc1, etc.) device can be opened
only once (until it is closed) and it is read-only. On read(2) and
select(2) the calling process is blocked until the next interrupt from
that RTC is received. Following the interrupt, the process can read a
long integer, of which the least significant byte contains a bit mask
encoding the types of interrupt that occurred, while the remaining 3
bytes contain the number of interrupts since the last read(2)."

So, it looks read() will always return only 4 bytes of useful info
(regardless of host being 32-bit/64-bit).

My questions are:

- Is the description in man page genuinely accurate?

- To me (but I am really an outsider to using RTC in applications),
this feature (blocking read()/select()) even looks very nice and
convenient, in all fairness. But I would like to ask you: Is this
feature used rarely or frequently by other libraries/tools/etc.? In
other words, is the feature "obscure" or "crucial" part of RTC kernel
support? Or, something in between?

- Does MC146818 support this feature?

Thanks a lot in advance for any response!

Aleksandar

P.S. for Arnd: I sent this message only to you, by mistake, around 15
min ago. Now I include everybody.

>        Arnd

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl
  2020-01-17 20:50                   ` Aleksandar Markovic
@ 2020-01-17 21:45                     ` Arnd Bergmann
  2020-01-17 21:54                     ` Alexandre Belloni
  1 sibling, 0 replies; 4+ messages in thread
From: Arnd Bergmann @ 2020-01-17 21:45 UTC (permalink / raw)
  To: Aleksandar Markovic
  Cc: Laurent Vivier, Filip Bozuta, Peter Maydell,
	Daniel P. Berrangé,
	Richard Henderson, qemu-devel, Max Filippov, amarkovic, philmd,
	Alexandre Belloni, linux-rtc

On Fri, Jan 17, 2020 at 9:50 PM Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:

> Alexandre (and Arnd too, or any other person knowledgeable in the area),
>
> I just need to clarify a couple of details with you, please.
>
> Firstly, here is what man page rtc(4) says:
>
> "The /dev/rtc (or /dev/rtc0, /dev/rtc1, etc.) device can be opened
> only once (until it is closed) and it is read-only. On read(2) and
> select(2) the calling process is blocked until the next interrupt from
> that RTC is received. Following the interrupt, the process can read a
> long integer, of which the least significant byte contains a bit mask
> encoding the types of interrupt that occurred, while the remaining 3
> bytes contain the number of interrupts since the last read(2)."
>
> So, it looks read() will always return only 4 bytes of useful info
> (regardless of host being 32-bit/64-bit).

It says "long integer", which is 64-bit on a 64-bit machine.

> My questions are:
>
> - Is the description in man page genuinely accurate?

Starting with linux-2.6.18, there is another possibility: If an
application asks for exactly four bytes on a 64-bit kernel,
it gets the lower four bytes, as it would on a 32-bit kernel.

This is a hack that was introduced for running 32-bit compat
tasks.

For any other size less than sizeof(long), the kernel reports
an EINVAL error, and for anything larger or equal to sizeof(long)
it attempts to output a long word.

> - To me (but I am really an outsider to using RTC in applications),
> this feature (blocking read()/select()) even looks very nice and
> convenient, in all fairness. But I would like to ask you: Is this
> feature used rarely or frequently by other libraries/tools/etc.? In
> other words, is the feature "obscure" or "crucial" part of RTC kernel
> support? Or, something in between?

> - Does MC146818 support this feature?

No idea, I'll leave these for Alexandre or someone else to answer.

      Arnd

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl
  2020-01-17 20:50                   ` Aleksandar Markovic
  2020-01-17 21:45                     ` Arnd Bergmann
@ 2020-01-17 21:54                     ` Alexandre Belloni
  1 sibling, 0 replies; 4+ messages in thread
From: Alexandre Belloni @ 2020-01-17 21:54 UTC (permalink / raw)
  To: Aleksandar Markovic
  Cc: Arnd Bergmann, Laurent Vivier, Filip Bozuta, Peter Maydell,
	Daniel P. Berrangé,
	Richard Henderson, qemu-devel, Max Filippov, amarkovic, philmd,
	linux-rtc

Hi,

Quick answers to your very good questions below:

On 17/01/2020 21:50:34+0100, Aleksandar Markovic wrote:
> Alexandre (and Arnd too, or any other person knowledgeable in the area),
> 
> I just need to clarify a couple of details with you, please.
> 
> Firstly, here is what man page rtc(4) says:
> 
> "The /dev/rtc (or /dev/rtc0, /dev/rtc1, etc.) device can be opened
> only once (until it is closed) and it is read-only. On read(2) and
> select(2) the calling process is blocked until the next interrupt from
> that RTC is received. Following the interrupt, the process can read a
> long integer, of which the least significant byte contains a bit mask
> encoding the types of interrupt that occurred, while the remaining 3
> bytes contain the number of interrupts since the last read(2)."
> 
> So, it looks read() will always return only 4 bytes of useful info
> (regardless of host being 32-bit/64-bit).
> 
> My questions are:
> 
> - Is the description in man page genuinely accurate?
> 

It is accurate. It is a mask of:

#define RTC_IRQF 0x80   /* Any of the following is active */
#define RTC_PF 0x40     /* Periodic interrupt */
#define RTC_AF 0x20     /* Alarm interrupt */
#define RTC_UF 0x10     /* Update interrupt for 1Hz RTC */

Which will most likely be RTC_IRQF | RTC_AF.

> - To me (but I am really an outsider to using RTC in applications),
> this feature (blocking read()/select()) even looks very nice and
> convenient, in all fairness. But I would like to ask you: Is this
> feature used rarely or frequently by other libraries/tools/etc.? In
> other words, is the feature "obscure" or "crucial" part of RTC kernel
> support? Or, something in between?
> 

Nobody is actually using the return value.

> - Does MC146818 support this feature?
> 

This feature is implemented in the RTC core so it is supported by all
RTCs that have alarms.

Internally, the value is an unsigned long and it is casted properly by
put_user in rtc_dev_read:

https://elixir.bootlin.com/linux/v5.4/source/drivers/rtc/dev.c#L178

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1579103618-20217-1-git-send-email-Filip.Bozuta@rt-rk.com>
     [not found] ` <1579103618-20217-9-git-send-email-Filip.Bozuta@rt-rk.com>
     [not found]   ` <CAK8P3a187rPhma7Q6o+hCF3h0=5MLZwh49+JqKt6BvVsAB1efQ@mail.gmail.com>
     [not found]     ` <ceaf44c0-fd6c-c280-7f95-7bc133553089@vivier.eu>
     [not found]       ` <CAK8P3a36KqWD4fKBLDpFhJg079bNdJDSDUAP2Zu_i1+H62Q6ZQ@mail.gmail.com>
     [not found]         ` <518d717d-9f1e-e00e-f2a9-df8861241d1c@rt-rk.com>
     [not found]           ` <cdcce2a3-00f5-f6d1-3083-dc36892ac5b4@vivier.eu>
     [not found]             ` <CAL1e-=i3-nYJMo6ptA7fdcK8r6P4vv20x2+LLV6BA9ELO8H53w@mail.gmail.com>
     [not found]               ` <CAL1e-=g8X___59zLPKLRjFNAP9bs3rVWhc8+OhMuF3TriBiynw@mail.gmail.com>
2020-01-16 12:00                 ` [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl Arnd Bergmann
2020-01-17 20:50                   ` Aleksandar Markovic
2020-01-17 21:45                     ` Arnd Bergmann
2020-01-17 21:54                     ` Alexandre Belloni

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git