sound: Simplify logic in dsp_io_ops()
ClosedPublic
Actions

Authored by christos on Thu, Nov 13, 2:34 PM.

Details

Reviewers

markj
emaste
kib

Commits

rGb4c32d67d40a: sound: Simplify logic in dsp_io_ops()

Summary

Use CHN_LOCK()/CHN_UNLOCK() directly, instead of
dsp_lock_chans()/dsp_unlock_chans(). These functions are useful when we
want to potentially lock both channels. Here we know which channel we
are locking, so we can just lock it directly. This way we get rid of the
prio variable as well.

Related to runpid again, there is no reason to assign it when
CHN_F_RUNNING is not set. channel->pid (as well as channel->comm) is
always assigned in dsp_chn_alloc().

Get rid of runpid. I do not see how we can end up with channel->pid
(td->td_proc->p_pid) not matching buf->uio_td->td_proc->p_pid.

Improve errno values.

not matching

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 68601
Build 65484: arc lint + arc unit

Event Timeline

christos created this revision.Thu, Nov 13, 2:34 PM

Herald added a subscriber: imp. · View Herald TranscriptThu, Nov 13, 2:34 PM

christos requested review of this revision.Thu, Nov 13, 2:34 PM

Harbormaster completed remote builds in B68601: Diff 166361.Thu, Nov 13, 2:34 PM

What is channel->pid? Where is it got assigned?

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

From the commit message:

channel->pid (as well as channel->comm) is always assigned in dsp_chn_alloc().

I will correct the message to say pcm_channel->pid, which is the actual struct's name.

In D53736#1227757, @christos wrote:

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

From the commit message:

channel->pid (as well as channel->comm) is always assigned in dsp_chn_alloc().

I will correct the message to say pcm_channel->pid, which is the actual struct's name.

This is not what I am asking about. Why do you need to save the pid, and how do you use it?

In D53736#1227774, @kib wrote:

In D53736#1227757, @christos wrote:

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

From the commit message:

channel->pid (as well as channel->comm) is always assigned in dsp_chn_alloc().

I will correct the message to say pcm_channel->pid, which is the actual struct's name.

This is not what I am asking about. Why do you need to save the pid, and how do you use it?

Both the pid and process name are mostly cosmetic, and are used in sndctl(8), /dev/sndstat and in some OSSv4 structures. It is just so that the user can easily tell which channel(s) belong to which processes.

In D53736#1227776, @christos wrote:

In D53736#1227774, @kib wrote:

In D53736#1227757, @christos wrote:

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

From the commit message:

channel->pid (as well as channel->comm) is always assigned in dsp_chn_alloc().

I will correct the message to say pcm_channel->pid, which is the actual struct's name.

This is not what I am asking about. Why do you need to save the pid, and how do you use it?

Both the pid and process name are mostly cosmetic, and are used in sndctl(8), /dev/sndstat and in some OSSv4 structures. It is just so that the user can easily tell which channel(s) belong to which processes.

So for instance, sharing the open file after fork, or passing it over unix domain socket, cause only a cosmetic issues, right?

In D53736#1227777, @kib wrote:

In D53736#1227776, @christos wrote:

In D53736#1227774, @kib wrote:

In D53736#1227757, @christos wrote:

In D53736#1227693, @kib wrote:

What is channel->pid? Where is it got assigned?

From the commit message:

channel->pid (as well as channel->comm) is always assigned in dsp_chn_alloc().

I will correct the message to say pcm_channel->pid, which is the actual struct's name.

This is not what I am asking about. Why do you need to save the pid, and how do you use it?

Both the pid and process name are mostly cosmetic, and are used in sndctl(8), /dev/sndstat and in some OSSv4 structures. It is just so that the user can easily tell which channel(s) belong to which processes.

So for instance, sharing the open file after fork, or passing it over unix domain socket, cause only a cosmetic issues, right?

Yes.

Bump.

What's the purpose of tracking the PID in the first place? What happens if the file descriptor is transferred over a unix socket to a different process, who then attempts I/O on it?

not matching

What does this mean?

In D53736#1230485, @markj wrote:

What's the purpose of tracking the PID in the first place? What happens if the file descriptor is transferred over a unix socket to a different process, who then attempts I/O on it?

Eh, sorry, I didn't read the discussion above.

markj added inline comments.Fri, Nov 21, 3:28 PM

sys/dev/sound/pcm/dsp.c
497	Does CHN_F_RUNNING serve any purpose?

In D53736#1230485, @markj wrote:

What does this mean?

Some leftover that I meant to delete. I have it removed already.

sys/dev/sound/pcm/dsp.c
497	IMHO no. It is only used inside the `CHN_F_MMAP_INVALID` flag in `dsp_mmap_single()`, but I want to understand it a bit more to make sure it's safe to delete.

markj accepted this revision.Fri, Nov 21, 3:42 PM

This revision is now accepted and ready to land.Fri, Nov 21, 3:42 PM

christos added inline comments.Fri, Nov 21, 3:47 PM

sys/dev/sound/pcm/dsp.c
497	My guess is `CHN_F_MMAP_INVALID` tests if the channel can be mmaped, and the reason it's defined as: #define CHN_F_MMAP_INVALID (CHN_F_DEAD \| CHN_F_RUNNING) Is because you cannot mmap a channel that is currently dead or already running. I'll try to find a way to get rid of this.