Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay
On (Mon) 14 Jul 2014 [18:12:46], Amit Shah wrote:
> On (Mon) 14 Jul 2014 [08:37:00], Jason Cooper wrote:
> > On Mon, Jul 14, 2014 at 10:05:19AM +0530, Amit Shah wrote:
> > > Some RNG devices may not be ready to give early randomness at probe()
> > > time, and hence lose out on the opportunity to contribute to system
> > > randomness at boot- or device hotplug- time.
> > >
> > > This commit schedules a delayed work item for such devices, and fetches
> > > early randomness after a delay. Currently the delay is 500ms, which is
> > > enough for the lone device that needs such treatment: virtio-rng.
> > >
> > > CC: Kees Cook
> > > CC: Jason Cooper
> > > CC: Herbert Xu
> > > Signed-off-by: Amit Shah
> > > ---
> > > drivers/char/hw_random/core.c | 20 +++-
> > > include/linux/hw_random.h | 8
> > > 2 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > > index c4419ea..2a765fd 100644
> > > --- a/drivers/char/hw_random/core.c
> > > +++ b/drivers/char/hw_random/core.c
> > > @@ -63,7 +63,7 @@ static size_t rng_buffer_size(void)
> > > return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
> > > }
> > >
> > > -static void add_early_randomness(struct hwrng *rng)
> > > +static void get_early_randomness(struct hwrng *rng)
> > > {
> > > unsigned char bytes[16];
> > > int bytes_read;
> > > @@ -79,6 +79,21 @@ static void add_early_randomness(struct hwrng *rng)
> > > add_device_randomness(bytes, bytes_read);
> > > }
> > >
> > > +static void sched_init_random(struct work_struct *work)
> > > +{
> > > + struct hwrng *rng = container_of(work, struct hwrng, dwork.work);
> > > +
> > > + get_early_randomness(rng);
> > > +}
> > > +
> > > +static void add_early_randomness(struct hwrng *rng)
> >
> > The add/get naming seems awkward in the above hunks.
>
> Yea; I felt that too. I thought of a do_add_early_randomness()
> instead, but that seemed awkward too. I forgot to mention I was
> planning on revisiting this naming for v1.
>
> > > +{
> > > + if (!(rng->flags & HWRNG_DELAY_READ_AT_INIT))
> > > + return get_early_randomness(rng);
> > > +
> > > + schedule_delayed_work(&rng->dwork, msecs_to_jiffies(500));
> > > +}
> > > +
> >
> > Perhaps instead of rng->flags and a hardcoded delay, we could have
> > rng->seed_delay = msecs_to_jiffies(500) in virtio-rng? Then you can
> > just call unconditionally:
> >
> > schedule_delayed_work(&rng->dwork, rng->seed_delay);
BTW I didn't want to make this call unconditional -- i.e. the existing
behaviour of in-line fetching of randomness for all devices but one
should not be affected.
If indeed people are OK with this being done by a delayed work item
for all the drivers, the code can get a bit simpler here.
> > I think that would be a more extensible solution should other drivers
> > show up with the same issue.
>
> Sounds like a good idea to me. Though, changes in core.c that
> increase the time in hwrng_register() or hwrng_init() may not get
> noticed by rng drivers and they may suddenly start failing for no
> apparent reason. Seems like a far stretch, though. Does anyone else
> have an opinion on this?
Herbert, do you have any preference?
Thanks,
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay
On Fri, Jul 18, 2014 at 02:26:26PM +0530, Amit Shah wrote: > > > Sounds like a good idea to me. Though, changes in core.c that > > increase the time in hwrng_register() or hwrng_init() may not get > > noticed by rng drivers and they may suddenly start failing for no > > apparent reason. Seems like a far stretch, though. Does anyone else > > have an opinion on this? > > Herbert, do you have any preference? So it's only virtio-rng that's a problem, right? How about if we abuse the scan hook in virtio and move the hwrng_register there? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/3] hw_random: allow RNG devices to give early randomness after a delay
On (Fri) 18 Jul 2014 [17:14:08], Herbert Xu wrote: > On Fri, Jul 18, 2014 at 02:26:26PM +0530, Amit Shah wrote: > > > > > Sounds like a good idea to me. Though, changes in core.c that > > > increase the time in hwrng_register() or hwrng_init() may not get > > > noticed by rng drivers and they may suddenly start failing for no > > > apparent reason. Seems like a far stretch, though. Does anyone else > > > have an opinion on this? > > > > Herbert, do you have any preference? > > So it's only virtio-rng that's a problem, right? How about if we > abuse the scan hook in virtio and move the hwrng_register there? Oops, I had completely missed the scan hook, and looks like it was added for exactly the purpose we want here (so we won't even be abusing it!) Thanks! I'll post the patches when the one to revert gets an upstream commit id. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 23/25] virtio: Replace DEFINE_PCI_DEVICE_TABLE macro use
We should prefer `struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet
kernel coding style guidelines. This issue was reported by checkpatch.
Signed-off-by: Benoit Taine
---
Tested by compilation without errors.
drivers/virtio/virtio_pci.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 101db3f..3d1463c 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -91,7 +91,7 @@ struct virtio_pci_vq_info
};
/* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
-static DEFINE_PCI_DEVICE_TABLE(virtio_pci_id_table) = {
+static const struct pci_device_id virtio_pci_id_table[] = {
{ PCI_DEVICE(0x1af4, PCI_ANY_ID) },
{ 0 }
};
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
We should prefer `const struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. This issue was reported by checkpatch. A simplified version of the semantic patch that makes this change is as follows (http://coccinelle.lip6.fr/): // @@ identifier i; declarer name DEFINE_PCI_DEVICE_TABLE; initializer z; @@ - DEFINE_PCI_DEVICE_TABLE(i) + const struct pci_device_id i[] = z; // I have 103 patches ready, and will only send a few for you to judge if it is useful enough, and to prevent from spamming too much. Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: > We should prefer `const struct pci_device_id` over > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. > This issue was reported by checkpatch. Honestly, I prefer the macro -- it stands-out more. Maybe the style guidelines and/or checkpatch should change instead? John -- John W. LinvilleSomeday the world will need a hero, and you linvi...@tuxdriver.com might be all we have. Be ready. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 09:28 -0700, James Bottomley wrote: > On Fri, 2014-07-18 at 17:26 +0200, Benoit Taine wrote: > > We should prefer `const struct pci_device_id` over > > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. > > This issue was reported by checkpatch. > > What kernel coding style? checkpatch isn't the arbiter of style, if > that's the only problem. > > The DEFINE_PCI macro was a well reasoned addition when it was added in > 2008. The problem was most people were getting the definition wrong. > When we converted away from CONFIG_HOTPLUG, having this DEFINE_ meant > that only one place needed changing instead of hundreds for PCI tables. > > The reason people were getting the PCI table wrong was mostly the init > section specifiers which are now gone, but it has enough underlying > utility (mostly constification) that I don't think we'd want to churn > the kernel hugely to make a change to struct pci_table and then have to > start detecting and fixing misuses. > > James > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html I would rather fix the misuses of the macro, than remove it. Could we possibly make checkpatch smart enough to tell when the macro is misused? Thanks, Jake ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: > On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: > > We should prefer `const struct pci_device_id` over > > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. > > This issue was reported by checkpatch. > > Honestly, I prefer the macro -- it stands-out more. Maybe the style > guidelines and/or checkpatch should change instead? The macro is horrid, no other bus has this type of thing just to save a few characters in typing, so why should PCI be "special" in this regard anymore? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, Jul 18, 2014 at 09:54:32AM -0700, James Bottomley wrote: > On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote: > > On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: > > > On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: > > > > We should prefer `const struct pci_device_id` over > > > > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. > > > > This issue was reported by checkpatch. > > > > > > Honestly, I prefer the macro -- it stands-out more. Maybe the style > > > guidelines and/or checkpatch should change instead? > > > > The macro is horrid, no other bus has this type of thing just to save a > > few characters in typing > > OK, so this is the macro: > > #define DEFINE_PCI_DEVICE_TABLE(_table) \ > const struct pci_device_id _table[] > > Could you explain what's so horrible? > > The reason it's useful today is that people forget the const (and > sometimes the [] making it a true table instead of a pointer). If you > use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it > wrongly (good) and you automatically get the correct annotations. We have almost 1000 more uses of the non-macro version than the "macro" version in the kernel today: $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l 262 $ git grep "const struct pci_device_id" | wc -l 1254 My big complaint is that we need to be consistant, either pick one or the other and stick to it. As the macro is the least used, it's easiest to fix up, and it also is more consistant with all other kernel subsystems which do not have such a macro. As there is no need for the __init macro mess anymore, there's no real need for the DEFINE_PCI_DEVICE_TABLE macro either. I think checkpatch will catch the use of non-const users for the id table already today, it catches lots of other uses like this already. > > , so why should PCI be "special" in this regard > > anymore? > > I think the PCI usage dwarfs most other bus types now, so you could turn > the question around. However, I don't think majority voting is a good > guide to best practise; lets debate the merits for their own sake. Not really "dwarf", USB is close with over 700 such structures: $ git grep "const struct usb_device_id" | wc -l 725 And i2c is almost just as big as PCI: $ git grep "const struct i2c_device_id" | wc -l 1223 So again, this macro is not consistent with the majority of PCI drivers, nor with any other type of "device id" declaration in the kernel, which is why I feel it should be removed. And finally, the PCI documentation itself says to not use this macro, so this isn't a "new" thing. From Documentation/PCI/pci.txt: The ID table is an array of struct pci_device_id entries ending with an all-zero entry. Definitions with static const are generally preferred. Use of the deprecated macro DEFINE_PCI_DEVICE_TABLE should be avoided. That wording went into the file last December, when we last talked about this and everyone in that discussion agreed to remove the macro for the above reasons. Consistency matters. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
> > We have almost 1000 more uses of the non-macro version than the "macro" > version in the kernel today: > $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l > 262 > $ git grep "const struct pci_device_id" | wc -l > 1254 did you check for non-const ones? just to see if we have any of the broken case in the tree :-) as for consistency, pci_dev vs usb_device :-P Dave. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Sat, Jul 19, 2014 at 07:14:12AM +1000, Dave Airlie wrote: > > > > We have almost 1000 more uses of the non-macro version than the "macro" > > version in the kernel today: > > $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l > > 262 > > $ git grep "const struct pci_device_id" | wc -l > > 1254 > > did you check for non-const ones? just to see if we have any of the > broken case in the tree :-) I didn't :) > as for consistency, pci_dev vs usb_device :-P Read farther down the email... ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote:
> On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote:
> > On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote:
> > > We should prefer `const struct pci_device_id` over
> > > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines.
> > > This issue was reported by checkpatch.
> > scripts/checkpatch.pl | 4 ++--
> > Honestly, I prefer the macro -- it stands-out more. Maybe the style
> > guidelines and/or checkpatch should change instead?
>
> The macro is horrid, no other bus has this type of thing just to save a
> few characters in typing, so why should PCI be "special" in this regard
> anymore?
I think it doesn't matter much.
The PCI_DEVICE and PCI_VDEVICE macro uses are somewhat similar
and are frequently used with PCI_DEVICE_TABLE, so there's some
commonality there.
The checkpatch message could be made --strict/CHK instead of
WARN so most people would never see it.
Of course it could be removed altogether too. I don't care.
---
(suggested patch is for -next)
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dc72a9b..754fbf2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3018,8 +3018,8 @@ sub process {
# check for uses of DEFINE_PCI_DEVICE_TABLE
if ($line =~ /\bDEFINE_PCI_DEVICE_TABLE\s*\(\s*(\w+)\s*\)\s*=/)
{
- if (WARN("DEFINE_PCI_DEVICE_TABLE",
-"Prefer struct pci_device_id over deprecated
DEFINE_PCI_DEVICE_TABLE\n" . $herecurr) &&
+ if (CHK("DEFINE_PCI_DEVICE_TABLE",
+ "Prefer struct pci_device_id over deprecated
DEFINE_PCI_DEVICE_TABLE\n" . $herecurr) &&
$fix) {
$fixed[$fixlinenr] =~
s/\b(?:static\s+|)DEFINE_PCI_DEVICE_TABLE\s*\(\s*(\w+)\s*\)\s*=\s*/static const
struct pci_device_id $1\[\] = /;
}
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
