Kernel Newbies archive on lore.kernel.org
 help / color / Atom feed
From: Jay Aurabind <jay.aurabind@gmail.com>
To: Valdis Kletnieks <valdis.kletnieks@vt.edu>
Cc: kernelnewbies <kernelnewbies@kernelnewbies.org>
Subject: Re: Using binary attributes for configuration sysfs entries
Date: Wed, 13 Mar 2019 14:38:30 +0530
Message-ID: <CAOsEZoi9cHa80jNr86oAQAhHozkK6iRhCcVz5bPiTk30ge8Qxg@mail.gmail.com> (raw)
In-Reply-To: <27151.1552460207@turing-police>

Dear Valdis,

Thank you for the response. Please see my inline queries:

On Wed, 13 Mar 2019 at 12:26, <valdis.kletnieks@vt.edu> wrote:
>
> On Wed, 13 Mar 2019 12:02:02 +0530, Jay Aurabind said:
>
> > For the corresponding sysfs interface, since there are a lot of
> > parameters, would it be justified to use the same binary format though
> > sysfs_create_binary_file() ? The rationale is that it would be easier
> > to simply pack all the config options in the struct and send it in
> > once rather than individually write 40 files. This is what the
> > attached patch follows. Interface is added only for reception
> > parameters as of now.
>
> Sysfs has a rule of "one value per file" - and saying "the one value is the
> config struct we read/write to the file" is stretching that quite a bit.   By
> the time you're doing all the marshaling of values in and out of this struct,
> there's no real benefit to do it via sysfs rather than the existing ioctl()
> call.
>
> Make sure you double-check that "the same binary format" means what you
> think it does when considering 32/64 bit architectures. While you're there, double
> check that the ioctl() works correctly for a 32-bit userspace program running
> on a 64-bit kernel - we have historically had an incredible number of API botches
> for that case.

The driver already provides a compat_ioctl that handles 32-bit
processes. That should suffice? However the actual configuration
structure has lots of enums in it. It isnt explicitly aligned. What
would be an ideal alignment factor? I've seen some code explicitly do
8 byte align while some others do 16 byte alight using 'u8
reserved[x]'. I suppose __attribute__ ((aligned (x))) is a better
option. Does it need to be aligned at the first place?

But as long as the ioctl interface isnt removed, changing the
configuration structure's representation is a bad idea I suppose. So I
should probably go full sysfs and remove ioctl calls altogether?

>
> Of course, it helps your case considerably if you can point at something else
> in sysfs and say "The Foobar 9934 driver already does this".. :)
>

I suppose this driver will serve that purpose:
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c#L1239
:)

> On the other hand, having 40 files is just a massive race condition waiting to
> happen, especially if the device has hardware weirdness that imply that certain
> sets of (say) 3 or 5 parameters have to be changed in unison.
>

The driver actually blocks upon a read (reception). So there does not
exist that possibility yet I think. But I should definitely check this
condition in sysfs_write so that I dont change the rx_config when
reception is active.


-- 

Thanks and Regards,
Jay

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-13  6:32 Jay Aurabind
2019-03-13  6:56 ` valdis.kletnieks
2019-03-13  9:08   ` Jay Aurabind [this message]
2019-03-13 14:25 ` Greg KH
2019-03-14 10:47   ` Jay Aurabind
2019-03-14 14:57     ` Greg KH
2019-03-14 17:41       ` Jay Aurabind

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOsEZoi9cHa80jNr86oAQAhHozkK6iRhCcVz5bPiTk30ge8Qxg@mail.gmail.com \
    --to=jay.aurabind@gmail.com \
    --cc=kernelnewbies@kernelnewbies.org \
    --cc=valdis.kletnieks@vt.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Kernel Newbies archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernelnewbies/0 kernelnewbies/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 kernelnewbies kernelnewbies/ https://lore.kernel.org/kernelnewbies \
		kernelnewbies@kernelnewbies.org kernelnewbies@archiver.kernel.org
	public-inbox-index kernelnewbies


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernelnewbies.kernelnewbies


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