Again, only now having time to relook at this.
Will re-rev and reconnect with Ashutosh offline since I've been absent on this for too long.
Thanks again Ashutosh for reviewing all these back in May.
On Wed, 2022-12-07 at 08:50 -0800, Dixit, Ashutosh wrote:
> On Tue, 06 Dec 2022 01:20:59 -0800, Alan Previn wrote:
> >
> > GuC log relay debugfs name for the control handle vs the actual relay
> > channel are vague. Fix them so it's obvious from the name.
>
> No real objection to anything here, just a couple of questions.
>
> >
> >
alan:snip
> > n_subbufs = intel_guc_log_relay_subbuf_count(log);
> >
> > - guc_log_relay_chan = relay_open("guc_log",
> > + guc_log_relay_chan = relay_open("guc_log_relay_chan",
>
> Is this a user visible name or just something internal? I.e. Is this a user
> visible file name?
>
It will be user visible. I think we need sudo though - need to double check that.
And in case i missed it earlier, the name change was solely to differentiate the
"relay channel" handle (that needs to be openned and read as per kernel relay-
channel framework) from all the other guc-log related debug-fs.
alan:snip
> > @@ -137,7 +137,7 @@ DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_subbuf_count_fops,
> > guc_log_relay_subbuf_count_get, NULL,
> > "%lld\n");
> >
> > -static int guc_log_relay_open(struct inode *inode, struct file *file)
> > +static int guc_log_relay_ctl_open(struct inode *inode, struct file *file)
>
> Again not objecting, but what is the purpose/thinking behind adding _ctl_
> to these function names? The previous names seemed fine?
>
Nothing wrong with the previous one - but since the existing relay logging tool
never worked anyways, i figure why not change the name to include "ctl" since we
are already using it for the tool to trigger flush by writing '1' to it,... if in
future we ever need more controls like 'write 2 for something else' or 'write 3
for something else' (i can think of a few examples but nothing urgent that needs to
be part of this immediate series).
I'm okay with changing back to original name - but for now will assume this new name
is okay - will connect offline.
