Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
On Fri, Nov 20, 2015 at 2:30 PM, Peter Ujfalusi wrote: > On 11/20/2015 02:24 PM, Andy Shevchenko wrote: >> On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann wrote: >>> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote: On 11/19/2015 01:25 PM, Arnd Bergmann wrote: >> >>> Another idea would be to remove the filter function from struct dma_chan_map >>> and pass the map through platform data >> >> Why not unified device properties? > > Is this some Windows/ACPI feature? Nope. Check drivers/base/property.c > Quick search gives mostly MSDN and > Windows10 related links. > > We only need dma_chan_map for platforms which has not been converted to DT and > still using legacy boot. Or platforms which can still boot in legacy mode. In > DT/ACPI mode we do not need this map at all. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
On Friday 20 November 2015 14:52:03 Peter Ujfalusi wrote:
>
> >> For legacy the filter function is pretty much needed to handle the
> >> differences
> >> between the platforms as not all of them does the filtering in a same way.
> >> So
> >> the first type of map would be feasible IMHO.
> >
> > It certainly makes the transition to a map table much easier.
>
> And the aim anyway is to convert everything to DT, right?
We won't be able to do that. Some architectures (avr32 and sh for instance)
use the dmaengine API but will likely never support DT. On ARM, at
least sa1100 is in the same category, probably also ep93xx and portions
of pxa, omap1 and davinci.
> > int dmam_register_platform_map(struct device *dev, dma_filter_fn filter,
> > struct dma_chan_map *map)
> > {
> > struct dma_map_list *list = kmalloc(sizeof(*list), GFP_KERNEL);
> >
> > if (!list)
> > return -ENOMEM;
> >
> > list->dev = dev;
> > list->filter = filter;
> > list->map = map;
> >
> > mutex_lock(&dma_map_mutex);
> > list_add(&dma_map_list, &list->node);
> > mutex_unlock(&dma_map_mutex);
> > }
> >
> > Now we can completely remove the dependency on the filter function
> > definition
> > from platform code and slave drivers.
>
> Sounds feasible for OMAP and daVinci and for others as well. I think
> I would go with this if someone asks my opinion
Ok.
> The core change to add the new API + the dma_map support should be pretty
> straight forward. It can live alongside with the old API and we can phase out
> the users of the old one.
> The legacy support would need more time since we need to modify the arch codes
> and the corresponding DMA drivers to get the map registered, but after that
> the remaining drivers can be converted to use the new API.
Right. It's not urgent and as long as we agree on the overall approach, we can
always do the platform support first and wait for the following merge window
before moving over the slave drivers.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
On 11/20/2015 12:58 PM, Arnd Bergmann wrote:
>>> That way the vast majority of drivers can use one of the two nice interfaces
>>> and the rest can be converted to use __dma_request_chan().
>>>
>>> On a related topic, we had in the past considered providing a way for
>>> platform code to register a lookup table of some sort, to associate
>>> a device/name pair with a configuration. That would let us use the
>>> simplified dma_request_slavechan(dev, name) pair everywhere. We could
>>> use the same method that we have for clk_register_clkdevs() or
>>> pinctrl_register_map().
>>>
>>> Something like either
>>>
>>> static struct dma_chan_map myplatform_dma_map[] = {
>>> { .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn,
>>> .arg = (void *)65, },
>>> { .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn,
>>> .arg = (void *)66, },
>>> };
>>>
>>> or
>>>
>>> static struct dma_chan_map myplatform_dma_map[] = {
>>> { .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0",
>>> .req = 65, },
>>> { .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0",
>>> .req = 66, },
>>
>> sa11x0-dma expects the fn_param as string :o
>
> Some of them do, but the new API requires changes in both the DMA master and
> slave drivers, so that could be changed if we wanted to, or we just allow
> both methods indefinitely and let sa11x0-dma pass the filterfn+data rather
> than
> a number.
Hrm, I would say that we need to push everyone to use the new API. sa11x0
should not be a big deal to fix IMHO and other users should be reasonably
simple to convert.
>>> };
>>
>> Basically we are deprecating the use of IORESOURCE_DMA?
>
> I thought we already had ;-)
For DT boot, yes. Not for the legacy boot.
>> For legacy the filter function is pretty much needed to handle the
>> differences
>> between the platforms as not all of them does the filtering in a same way. So
>> the first type of map would be feasible IMHO.
>
> It certainly makes the transition to a map table much easier.
And the aim anyway is to convert everything to DT, right?
>>> we could even allow a combination of the two, so the simple case just
>>> specifies
>>> master and req number, which requires changes to the dmaengine driver, but
>>> we could
>>> also do a mass-conversion to the .filter/.arg variant.
>>
>> This will get rid of the need for the fn and fn_param parameters when
>> requesting dma channel, but it will not get rid of the exported function from
>> the dma engine drivers since in arch code we need to have visibility to the
>> filter_fn.
>
> Correct. A lot of dmaengine drivers already need to be built-in so the
> platform
> code can put a pointer to the filter function, so it would not be worse for
> them.
>
> Another idea would be to remove the filter function from struct dma_chan_map
> and pass the map through platform data to the dmaengine driver, which then
> registers it to the core along with the mask. Something like:
>
> /* platform code */
> static struct dma_chan_map oma_dma_map[] = {
> { .devname = "omap-aes0", .slave = "tx", .arg = (void *)65, },
> { .devname = "omap-aes0", .slave = "rx", .arg = (void *)66, },
> ...
> {},
> };
>
> static struct omap_system_dma_plat_info dma_plat_info __initdata = {
> .dma_map = &oma_dma_map,
> ...
> };
>
> machine_init(void)
> {
> ...
> platform_device_register_data(NULL, "omap-dma-engine", 0,
> &dma_plat_info, sizeof(dma_plat_info);
> ...
> }
>
> /* dmaengine driver */
>
> static int omap_dma_probe(struct platform_device *pdev)
> {
> struct omap_system_dma_plat_info *pdata = dev_get_platdata(&pdev->dev);
> ...
>
> dmam_register_platform_map(&pdev->dev, omap_dma_filter_fn,
> pdata->dma_map);
> }
>
> /* dmaengine core */
>
> struct dma_map_list {
> struct list_head node;
> struct device *master;
> dma_filter_fn filter;
> struct dma_chan_map *map;
> };
>
> static LIST_HEAD(dma_map_list);
> static DEFINE_MUTEX(dma_map_mutex);
>
> int dmam_register_platform_map(struct device *dev, dma_filter_fn filter,
> struct dma_chan_map *map)
> {
> struct dma_map_list *list = kmalloc(sizeof(*list), GFP_KERNEL);
>
> if (!list)
> return -ENOMEM;
>
> list->dev = dev;
> list->filter = filter;
> list->map = map;
>
> mutex_lock(&dma_map_mutex);
> list_add(&dma_map_list, &list->node);
> mutex_unlock(&dma_map_mutex);
> }
>
> Now we can completely remove the dependency on the filter function definition
> from platform code and slave drivers.
Sounds feasible for OMAP and daVinci and for others as well. I think ;)
I would go with this if someone asks my opinion :D
The core change to add the new API + the dma_map support should be pretty
straight forward. It can live alongside with the old API and we can phase out
the users of the old one.
The legacy support would need more time
Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
On 11/20/2015 02:24 PM, Andy Shevchenko wrote: > On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann wrote: >> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote: >>> On 11/19/2015 01:25 PM, Arnd Bergmann wrote: > >> Another idea would be to remove the filter function from struct dma_chan_map >> and pass the map through platform data > > Why not unified device properties? Is this some Windows/ACPI feature? Quick search gives mostly MSDN and Windows10 related links. We only need dma_chan_map for platforms which has not been converted to DT and still using legacy boot. Or platforms which can still boot in legacy mode. In DT/ACPI mode we do not need this map at all. -- Péter -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann wrote: > On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote: >> On 11/19/2015 01:25 PM, Arnd Bergmann wrote: > Another idea would be to remove the filter function from struct dma_chan_map > and pass the map through platform data Why not unified device properties? -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] KEYS: Exposing {a,}symmetric key ops to userspace and other bits
Hi Marcel, Mimi, Tadeus,
I want to consider adding or doing the following bits to the keyrings
facility, aiming for the next merge window:
(*) Bring in the patches that I posted to change how the trust model on a
keyring works.
The model will then be that keys aren't automatically marked trusted, but
linking a key into a keyring that is marked trusted-only will validate
the key against the contents of the keyring before permitting its
addition.
Note that we can then vary the policy on a per-keyring basis.
(*) Add Mimi's patches to allow keys/keyrings to be marked undeletable. This
is for the purpose of creating blacklists and to prevent people from
removing entries in the blacklist. Note that only the kernel can create
a blacklist - we don't want userspace generating them as a way to take up
kernel space.
I think the right way to do this is to not allow marked keys to be
unlinked from marked keyrings, but to allow marked keys to be unlinked
from ordinary keyrings.
The reason the 'keep' mark is required on individual keys is to prevent
the keys from being directly revoked, expired or invalidated by keyctl
without reference to the keyring. Marked keys that are set expirable
when they're created will still expire and be subsequently removed and if
a marked key or marked keyring loses all its references it still gets
gc'd.
(*) Provide KEYCTL_{SIGN,VERIFY,ENCRYPT,DECRYPT} operations for use with
asymmetric keys, allowing offload to hardware or use of the crypto
routines for a software fallback.
One question is as to how to set parameters. The key will be specified
by a key ID and this will set the crypto algorithm (eg. RSA, DSA, ECDSA,
etc.) and the key size (eg. RSA-4096), but other parameters will need to
be supplied such as:
- Hash type. I'm expecting the hash value to be passed through this
interface not the data-to-be-hashed, but the type may need to be known
for other purposes.
- Password to decrypt the private key. I'm not sure whether this should
be presented at the point of key usage or the point of key
instantiation. The former means that you don't have an unsecured key
sitting around in the kernel.
Another question is what form the data should be presented. In many
ways, I would favour raw data with internal metadata attached as
appropriate by userspace (eg. the hash algorithm OID included in a
signature as per RFC4880 sec 5.2.2). I would certainly rather avoid any
ASN.1 or PGP encodings in this interface.
One problem we have is that we only have four arguments to play with, one
of which has to represent the key ID, but we need two buffers, two buffer
lengths and some options per operation. However, we could include the
buffer lengths inside the options maybe:
keyctl_sign(int key, const char *options, const void *data,
void *buffer);
Another option is to allow a key to be queried for the buffer sizes and
always require that amount of data - maybe something like:
struct keyctl_asymmetric_info {
unsigned encrypted_data_size;
unsigned decrypted_data_size;
unsigned signature_size;
unsigned signed_data_size;
} info;
keyctl_query_asymmetric(key, &info);
Possibly these values will all be the same, so we might only need get one
value back. I'm assuming here that userspace would do the dressing up of
the data for signing with whatever metadata and padding is required.
(*) In reference to the above, potentially provide a KEYCTL_KEY_UNLOCK that
takes a key and password and gives you another key that has the private
key unlocked that you can use temporarily and then discard. I'm not sure
how best to manage *hardware* private keys though - and I suspect that
will be hardware dependent.
(*) A TPM asymmetric key subtype that allows access to asymmetric keys stored
in a TPM.
(*) Provide KEYCTL_SEAL_KEY for sealing an asymmetric key to hardware.
(*) Add a symmetric key type that acts as a container for a symmteric key,
using either hardware or software, to be accessible through AF_ALG.
(*) Provide a way to generate a new symmetric key, encrypting it with an
asymmetric key inside the kernel.
Again, how to parameterise is probably a tricky question.
(*) Sort out the KEYCTL_UPDATE mess with trusted and encrypted keys.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
> >> dma_request_channel(mask); /* memcpy. etc, non slave mostly */
> >>
> >> Not sure how to name this as reusing existing (good, descriptive) function
> >> names would mean changes all over the kernel to start off this.
> >>
> >> Not used and
> >> request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
> >> request_dma();
> >> dma_channel_request();
> >
> > dma_request_slavechan();
> > dma_request_slave();
> > dma_request_mask();
>
> Let me think aloud here a bit...
> 1. To request slave channel which will return you the channel your device is
> bind via DT/ACPI or the platform map table you propose later:
>
> dma_request_chan(struct device *dev, const char *name);
>
> 2. To request a channel (any channel) matching with the capabilities the
> driver needs, like memcpy, memset, etc:
>
> #define dma_request_chan_by_mask(mask) __dma_request_chan_by_mask(&(mask))
> __dma_request_chan_by_mask(const dma_cap_mask_t *mask);
>
> I think the dma_request_chan() does not need mask to be passed, since via this
> we request a specific channel which has been defined/set by DT/ACPI or the
> lookup map. We could add a mask parameter which could be used to sanity check
> the channel we got against the capabilities the driver needs from the channel.
> We currently do this in the drivers where the author wanted to make sure that
> the channel is capable of what it should be capable.
>
> So two API to request channel:
> struct dma_chan *dma_request_chan(struct device *dev, const char *name);
> struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask);
>
> Both will return with the valid channel pointer or in case of failure with
> ERR_PTR().
>
> We need to go through the code in regards to return codes also to have sane
> mapping.
Right.
> > That way the vast majority of drivers can use one of the two nice interfaces
> > and the rest can be converted to use __dma_request_chan().
> >
> > On a related topic, we had in the past considered providing a way for
> > platform code to register a lookup table of some sort, to associate
> > a device/name pair with a configuration. That would let us use the
> > simplified dma_request_slavechan(dev, name) pair everywhere. We could
> > use the same method that we have for clk_register_clkdevs() or
> > pinctrl_register_map().
> >
> > Something like either
> >
> > static struct dma_chan_map myplatform_dma_map[] = {
> > { .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn,
> > .arg = (void *)65, },
> > { .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn,
> > .arg = (void *)66, },
> > };
> >
> > or
> >
> > static struct dma_chan_map myplatform_dma_map[] = {
> > { .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0",
> > .req = 65, },
> > { .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0",
> > .req = 66, },
>
> sa11x0-dma expects the fn_param as string :o
Some of them do, but the new API requires changes in both the DMA master and
slave drivers, so that could be changed if we wanted to, or we just allow
both methods indefinitely and let sa11x0-dma pass the filterfn+data rather than
a number.
> > };
>
> Basically we are deprecating the use of IORESOURCE_DMA?
I thought we already had ;-)
> For legacy the filter function is pretty much needed to handle the differences
> between the platforms as not all of them does the filtering in a same way. So
> the first type of map would be feasible IMHO.
It certainly makes the transition to a map table much easier.
> > we could even allow a combination of the two, so the simple case just
> > specifies
> > master and req number, which requires changes to the dmaengine driver, but
> > we could
> > also do a mass-conversion to the .filter/.arg variant.
>
> This will get rid of the need for the fn and fn_param parameters when
> requesting dma channel, but it will not get rid of the exported function from
> the dma engine drivers since in arch code we need to have visibility to the
> filter_fn.
Correct. A lot of dmaengine drivers already need to be built-in so the platform
code can put a pointer to the filter function, so it would not be worse for
them.
Another idea would be to remove the filter function from struct dma_chan_map
and pass the map through platform data to the dmaengine driver, which then
registers it to the core along with the mask. Something like:
/* platform code */
static struct dma_chan_map oma_dma_map[] = {
{ .devname = "omap-aes0", .slave = "tx", .arg = (void *)65, },
{ .devname = "omap-aes0", .slave = "rx", .arg = (void *)66, },
...
{},
};
static struct omap_system_dma_plat_info dma_plat_info __initdata = {
.dma_map = &oma_dma_map,
...
};
machine_init(void)
{
...
platform_device_register_data(NULL, "omap-dma-engine", 0,
&dma_pla
Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
>> If we have two main APIs, one to request slave channels and one to get any
>> channel with given capability
>> dma_request_slave_channel(NULL, NULL, &mask, fn, fn_param); /* Legacy slave
>> */
>> dma_request_slave_channel(dev, name, NULL, NULL, NULL); /* DT/ACPI, current
>>slave */
>> dma_request_slave_channel(dev, name, &mask, fn, fn_param); /* current
>> compat*/
>>
>> This way we can omit the mask also in cases when the client only want to get
>> DMA_SLAVE, we can just build up the mask within the function. If the mask is
>> provided we would copy the bits from the provided mask, so for example if you
>> want to have DMA_SLAVE+DMA_CYCLIC, the driver only needs to pass DMA_CYCLIC,
>> the DMA_SLAVE is going to be set anyways.
>
> I think it's more logical here to have mask=NULL mean that we want DMA_SLAVE,
> but otherwise pass the full mask as DMA_SLAVE|DMA_CYCLIC etc.
Yep, could be, while I would write the core part to set the DMA_SLAVE
unconditionally anyways. If the API say it is dma_request_slavechan() it is
expected to get channel which is capable of DMA_SLAVE.
>> dma_request_channel(mask); /* memcpy. etc, non slave mostly */
>>
>> Not sure how to name this as reusing existing (good, descriptive) function
>> names would mean changes all over the kernel to start off this.
>>
>> Not used and
>> request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
>> request_dma();
>> dma_channel_request();
>
> dma_request_slavechan();
> dma_request_slave();
> dma_request_mask();
Let me think aloud here a bit...
1. To request slave channel which will return you the channel your device is
bind via DT/ACPI or the platform map table you propose later:
dma_request_chan(struct device *dev, const char *name);
2. To request a channel (any channel) matching with the capabilities the
driver needs, like memcpy, memset, etc:
#define dma_request_chan_by_mask(mask) __dma_request_chan_by_mask(&(mask))
__dma_request_chan_by_mask(const dma_cap_mask_t *mask);
I think the dma_request_chan() does not need mask to be passed, since via this
we request a specific channel which has been defined/set by DT/ACPI or the
lookup map. We could add a mask parameter which could be used to sanity check
the channel we got against the capabilities the driver needs from the channel.
We currently do this in the drivers where the author wanted to make sure that
the channel is capable of what it should be capable.
So two API to request channel:
struct dma_chan *dma_request_chan(struct device *dev, const char *name);
struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask);
Both will return with the valid channel pointer or in case of failure with
ERR_PTR().
We need to go through the code in regards to return codes also to have sane
mapping.
>
>> All in all, not sure which way would be better...
>
> I think I would prefer the simplest API to have only the dev+name
> arguments, as we tend to move that way for all platforms anyway, and it
> seems silly to have all drivers pass three NULL arguments all the time.
> At the moment, there are 139 references to dma_request_slave_channel_*
> in the kernel, and only 46 of them are dma_request_slave_channel_compat.
> Out of those 46, a couple can already be converted back to use
> dma_request_slave_channel() because the platform now only supports
> devicetree based boots and will not go back to platform data.
>
> How about something like
>
> extern struct dma_chan *
> __dma_request_chan(struct device *dev, const char *name,
> const dma_cap_mask_t *mask, dma_filter_fn fn, void
> *fn_param);
>
> static inline struct dma_chan *
> dma_request_slavechan(struct device *dev, const char *name)
> {
> return __dma_request_chan(dev, name, NULL, NULL, NULL);
> }
>
> static inline struct dma_chan *
> dma_request_chan(const dma_cap_mask_t *mask)
> {
> return __dma_request_chan(NULL, NULL, mask, NULL, NULL);
> }
>
> That way the vast majority of drivers can use one of the two nice interfaces
> and the rest can be converted to use __dma_request_chan().
>
> On a related topic, we had in the past considered providing a way for
> platform code to register a lookup table of some sort, to associate
> a device/name pair with a configuration. That would let us use the
> simplified dma_request_slavechan(dev, name) pair everywhere. We could
> use the same method that we have for clk_register_clkdevs() or
> pinctrl_register_map().
>
> Something like either
>
> static struct dma_chan_map myplatform_dma_map[] = {
> { .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn,
> .arg = (void *)65, },
> { .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn,
> .arg = (void *)66, },
> };
>
> or
>
> static struct dma_chan_map myplatform_dma_map[] = {
> { .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0",
