* linux-next: manual merge of the sound-asoc tree with the crypto tree @ 2020-05-12 4:49 Stephen Rothwell 2020-05-12 16:22 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Stephen Rothwell @ 2020-05-12 4:49 UTC (permalink / raw) To: Mark Brown, Liam Girdwood, Herbert Xu, Linux Crypto List Cc: Linux Next Mailing List, Linux Kernel Mailing List, Eric Biggers, Arnd Bergmann [-- Attachment #1: Type: text/plain, Size: 773 bytes --] Hi all, Today's linux-next merge of the sound-asoc tree got a conflict in: sound/soc/codecs/cros_ec_codec.c between commit: 85fc78b80f15 ("ASoC: cros_ec_codec: use crypto_shash_tfm_digest()") from the crypto tree and commit: a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically") from the sound-asoc tree. I fixed it up (I just used the former) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree 2020-05-12 4:49 linux-next: manual merge of the sound-asoc tree with the crypto tree Stephen Rothwell @ 2020-05-12 16:22 ` Mark Brown 2020-05-12 16:36 ` Eric Biggers 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2020-05-12 16:22 UTC (permalink / raw) To: Stephen Rothwell Cc: Liam Girdwood, Herbert Xu, Linux Crypto List, Linux Next Mailing List, Linux Kernel Mailing List, Eric Biggers, Arnd Bergmann [-- Attachment #1: Type: text/plain, Size: 1036 bytes --] On Tue, May 12, 2020 at 02:49:49PM +1000, Stephen Rothwell wrote: > Hi all, > > Today's linux-next merge of the sound-asoc tree got a conflict in: > > sound/soc/codecs/cros_ec_codec.c > > between commit: > > 85fc78b80f15 ("ASoC: cros_ec_codec: use crypto_shash_tfm_digest()") > Oh, this is the first I've heard of that patch :( > from the crypto tree and commit: > a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically") > from the sound-asoc tree. > I fixed it up (I just used the former) and can carry the fix as > necessary. This is now fixed as far as linux-next is concerned, but any > non trivial conflicts should be mentioned to your upstream maintainer > when your tree is submitted for merging. You may also want to consider > cooperating with the maintainer of the conflicting tree to minimise any > particularly complex conflicts. That doesn't seem ideal - Eric, Herbert can we get a branch with the crypto patches in them to pull into the ASoC tree or something? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree 2020-05-12 16:22 ` Mark Brown @ 2020-05-12 16:36 ` Eric Biggers 2020-05-12 17:08 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Eric Biggers @ 2020-05-12 16:36 UTC (permalink / raw) To: Mark Brown Cc: Stephen Rothwell, Liam Girdwood, Herbert Xu, Linux Crypto List, Linux Next Mailing List, Linux Kernel Mailing List, Arnd Bergmann On Tue, May 12, 2020 at 05:22:05PM +0100, Mark Brown wrote: > On Tue, May 12, 2020 at 02:49:49PM +1000, Stephen Rothwell wrote: > > Hi all, > > > > Today's linux-next merge of the sound-asoc tree got a conflict in: > > > > sound/soc/codecs/cros_ec_codec.c > > > > between commit: > > > > 85fc78b80f15 ("ASoC: cros_ec_codec: use crypto_shash_tfm_digest()") > > > > Oh, this is the first I've heard of that patch :( > > > from the crypto tree and commit: > > > a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically") > > > from the sound-asoc tree. I Cc'ed it to the people listed in MAINTAINERS for "CHROMEOS EC CODEC DRIVER". I guess wasn't enough and I should have added alsa-devel@alsa-project.org too? > > > I fixed it up (I just used the former) and can carry the fix as > > necessary. This is now fixed as far as linux-next is concerned, but any > > non trivial conflicts should be mentioned to your upstream maintainer > > when your tree is submitted for merging. You may also want to consider > > cooperating with the maintainer of the conflicting tree to minimise any > > particularly complex conflicts. > > That doesn't seem ideal - Eric, Herbert can we get a branch with the > crypto patches in them to pull into the ASoC tree or something? We should just drop one of the patches. If you just want to eliminate the compiler warning about stack usage in wov_hotword_model_put(), then my patch in cryptodev would be better, as it moves the stack allocation into another function in another file. Alternatively, if you actually need to reduce the total stack usage (across all functions), then the kmalloc() patch in sound-asoc would be better. Which do you prefer? - Eric ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree 2020-05-12 16:36 ` Eric Biggers @ 2020-05-12 17:08 ` Mark Brown 2020-05-12 20:08 ` Eric Biggers 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2020-05-12 17:08 UTC (permalink / raw) To: Eric Biggers Cc: Stephen Rothwell, Liam Girdwood, Herbert Xu, Linux Crypto List, Linux Next Mailing List, Linux Kernel Mailing List, Arnd Bergmann [-- Attachment #1: Type: text/plain, Size: 1365 bytes --] On Tue, May 12, 2020 at 09:36:32AM -0700, Eric Biggers wrote: > On Tue, May 12, 2020 at 05:22:05PM +0100, Mark Brown wrote: > > > from the crypto tree and commit: > > > a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically") > > > from the sound-asoc tree. > I Cc'ed it to the people listed in MAINTAINERS for "CHROMEOS EC CODEC DRIVER". > I guess wasn't enough and I should have added alsa-devel@alsa-project.org too? Yeah, you generally need to make sure the subsystem maintainers are included as well as individual driver maintainers. > > That doesn't seem ideal - Eric, Herbert can we get a branch with the > > crypto patches in them to pull into the ASoC tree or something? > We should just drop one of the patches. > If you just want to eliminate the compiler warning about stack usage in > wov_hotword_model_put(), then my patch in cryptodev would be better, as it moves > the stack allocation into another function in another file. > Alternatively, if you actually need to reduce the total stack usage (across all > functions), then the kmalloc() patch in sound-asoc would be better. Well, reducing the stack usage overall seems nicer overall - heads off future problems if the struct grows or something, and if we still end up allocating just as much on the stack then we'll have trouble at runtime anyway - does that make sense? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree 2020-05-12 17:08 ` Mark Brown @ 2020-05-12 20:08 ` Eric Biggers 2020-05-12 20:31 ` Arnd Bergmann 2020-05-13 2:52 ` Herbert Xu 0 siblings, 2 replies; 8+ messages in thread From: Eric Biggers @ 2020-05-12 20:08 UTC (permalink / raw) To: Mark Brown, Herbert Xu Cc: Stephen Rothwell, Liam Girdwood, Linux Crypto List, Linux Next Mailing List, Linux Kernel Mailing List, Arnd Bergmann On Tue, May 12, 2020 at 06:08:01PM +0100, Mark Brown wrote: > On Tue, May 12, 2020 at 09:36:32AM -0700, Eric Biggers wrote: > > On Tue, May 12, 2020 at 05:22:05PM +0100, Mark Brown wrote: > > > > > from the crypto tree and commit: > > > > > a1304cba816e ("ASoC: cros_ec_codec: allocate shash_desc dynamically") > > > > > from the sound-asoc tree. > > > I Cc'ed it to the people listed in MAINTAINERS for "CHROMEOS EC CODEC DRIVER". > > I guess wasn't enough and I should have added alsa-devel@alsa-project.org too? > > Yeah, you generally need to make sure the subsystem maintainers are > included as well as individual driver maintainers. > > > > That doesn't seem ideal - Eric, Herbert can we get a branch with the > > > crypto patches in them to pull into the ASoC tree or something? > > > We should just drop one of the patches. > > > If you just want to eliminate the compiler warning about stack usage in > > wov_hotword_model_put(), then my patch in cryptodev would be better, as it moves > > the stack allocation into another function in another file. > > > Alternatively, if you actually need to reduce the total stack usage (across all > > functions), then the kmalloc() patch in sound-asoc would be better. > > Well, reducing the stack usage overall seems nicer overall - heads off > future problems if the struct grows or something, and if we still end up > allocating just as much on the stack then we'll have trouble at runtime > anyway - does that make sense? If you're concerned about total stack usage, then my recommendation is that Herbert drops my patch "ASoC: cros_ec_codec: use crypto_shash_tfm_digest()" from cryptodev, and you keep the patch "ASoC: cros_ec_codec: allocate shash_desc dynamically" in sound-asoc. For later: if SHASH_DESC_ON_STACK is causing problems, we really ought to find a better solution, since lots of users are using this macro. A version of crypto_shash_tfm_digest() that falls back to heap allocation if the descsize is too large would be possible, but that wouldn't fully solve the problem since some users do incremental hashing. - Eric ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree 2020-05-12 20:08 ` Eric Biggers @ 2020-05-12 20:31 ` Arnd Bergmann 2020-05-14 13:21 ` Ard Biesheuvel 2020-05-13 2:52 ` Herbert Xu 1 sibling, 1 reply; 8+ messages in thread From: Arnd Bergmann @ 2020-05-12 20:31 UTC (permalink / raw) To: Eric Biggers Cc: Mark Brown, Herbert Xu, Stephen Rothwell, Liam Girdwood, Linux Crypto List, Linux Next Mailing List, Linux Kernel Mailing List On Tue, May 12, 2020 at 10:08 PM Eric Biggers <ebiggers@kernel.org> wrote: > On Tue, May 12, 2020 at 06:08:01PM +0100, Mark Brown wrote: > > For later: if SHASH_DESC_ON_STACK is causing problems, we really ought to find a > better solution, since lots of users are using this macro. A version of > crypto_shash_tfm_digest() that falls back to heap allocation if the descsize is > too large would be possible, but that wouldn't fully solve the problem since > some users do incremental hashing. It's hard to know how many of the users of SHASH_DESC_ON_STACK() are likely to cause problems, as multiple factors are involved: - this one triggered the warning because it was on the stack of a function that got inlined into another that has other large variables. Whether it got inlined makes little difference to the stack usage, but does make a difference to warning about it. - generally the structure is larger than we like it, especially on architectures with 128 byte CRYPTO_MINALIGN like ARM. This actually got worse because of b68a7ec1e9a3 ("crypto: hash - Remove VLA usage"), as the stack usage is now always the maximum of all hashes where it used to be specific to the hash that was actually used and could be smaller - the specific instance in calculate_sha256() feels a bit silly, as this function allocates a tfm and a descriptor, runs the digest and then frees both again. I don't know how common this pattern is, but it seems a higher-level abstraction might be helpful anyway. Arnd ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree 2020-05-12 20:31 ` Arnd Bergmann @ 2020-05-14 13:21 ` Ard Biesheuvel 0 siblings, 0 replies; 8+ messages in thread From: Ard Biesheuvel @ 2020-05-14 13:21 UTC (permalink / raw) To: Arnd Bergmann Cc: Eric Biggers, Mark Brown, Herbert Xu, Stephen Rothwell, Liam Girdwood, Linux Crypto List, Linux Next Mailing List, Linux Kernel Mailing List On Tue, 12 May 2020 at 22:31, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, May 12, 2020 at 10:08 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, May 12, 2020 at 06:08:01PM +0100, Mark Brown wrote: > > > > For later: if SHASH_DESC_ON_STACK is causing problems, we really ought to find a > > better solution, since lots of users are using this macro. A version of > > crypto_shash_tfm_digest() that falls back to heap allocation if the descsize is > > too large would be possible, but that wouldn't fully solve the problem since > > some users do incremental hashing. > > It's hard to know how many of the users of SHASH_DESC_ON_STACK() are > likely to cause problems, as multiple factors are involved: > > - this one triggered the warning because it was on the stack of a function > that got inlined into another that has other large variables. Whether it > got inlined makes little difference to the stack usage, but does make a > difference to warning about it. > > - generally the structure is larger than we like it, especially on architectures > with 128 byte CRYPTO_MINALIGN like ARM. This actually got worse > because of b68a7ec1e9a3 ("crypto: hash - Remove VLA usage"), as > the stack usage is now always the maximum of all hashes where it used > to be specific to the hash that was actually used and could be smaller > > - the specific instance in calculate_sha256() feels a bit silly, as this > function allocates a tfm and a descriptor, runs the digest and then > frees both again. I don't know how common this pattern is, but > it seems a higher-level abstraction might be helpful anyway. > We are trying to move to crypto library interfaces for non-performance critical uses of hashes where the algorithm is known at compile time, and this is a good example of that pattern. IOW, this code should just call the sha256_init/update/final routines directly. I'll send out a patch. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: linux-next: manual merge of the sound-asoc tree with the crypto tree 2020-05-12 20:08 ` Eric Biggers 2020-05-12 20:31 ` Arnd Bergmann @ 2020-05-13 2:52 ` Herbert Xu 1 sibling, 0 replies; 8+ messages in thread From: Herbert Xu @ 2020-05-13 2:52 UTC (permalink / raw) To: Eric Biggers Cc: Mark Brown, Stephen Rothwell, Liam Girdwood, Linux Crypto List, Linux Next Mailing List, Linux Kernel Mailing List, Arnd Bergmann On Tue, May 12, 2020 at 01:08:05PM -0700, Eric Biggers wrote: > > If you're concerned about total stack usage, then my recommendation is that > Herbert drops my patch "ASoC: cros_ec_codec: use crypto_shash_tfm_digest()" > from cryptodev, and you keep the patch > "ASoC: cros_ec_codec: allocate shash_desc dynamically" in sound-asoc. OK I'll drop this patch. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-05-14 13:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-12 4:49 linux-next: manual merge of the sound-asoc tree with the crypto tree Stephen Rothwell 2020-05-12 16:22 ` Mark Brown 2020-05-12 16:36 ` Eric Biggers 2020-05-12 17:08 ` Mark Brown 2020-05-12 20:08 ` Eric Biggers 2020-05-12 20:31 ` Arnd Bergmann 2020-05-14 13:21 ` Ard Biesheuvel 2020-05-13 2:52 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).