LinuxLists.cc - [PATCH 0/7] stm class/intel

[permalink] [raw]

Subject: [PATCH 3/7] intel_th: Constify all drvdata references

Anything that deals with drvdata structures should leave them intact.
Reflect this in function signatures.

Signed-off-by: Alexander Shishkin <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/hwtracing/intel_th/core.c | 2 +-
drivers/hwtracing/intel_th/intel_th.h | 6 +++---
drivers/hwtracing/intel_th/pci.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
index c9ac3dc65113..24d0c974bfd5 100644
--- a/drivers/hwtracing/intel_th/core.c
+++ b/drivers/hwtracing/intel_th/core.c
@@ -844,7 +844,7 @@ static irqreturn_t intel_th_irq(int irq, void *data)
* @irq: irq number
*/
struct intel_th *
-intel_th_alloc(struct device *dev, struct intel_th_drvdata *drvdata,
+intel_th_alloc(struct device *dev, const struct intel_th_drvdata *drvdata,
struct resource *devres, unsigned int ndevres)
{
int err, r, nr_mmios = 0;
diff --git a/drivers/hwtracing/intel_th/intel_th.h b/drivers/hwtracing/intel_th/intel_th.h
index 5fe694708b7a..05fa2dab37d1 100644
--- a/drivers/hwtracing/intel_th/intel_th.h
+++ b/drivers/hwtracing/intel_th/intel_th.h
@@ -74,7 +74,7 @@ struct intel_th_drvdata {
*/
struct intel_th_device {
struct device dev;
- struct intel_th_drvdata *drvdata;
+ const struct intel_th_drvdata *drvdata;
struct resource *resource;
unsigned int num_resources;
unsigned int type;
@@ -224,7 +224,7 @@ static inline struct intel_th *to_intel_th(struct intel_th_device *thdev)
}

struct intel_th *
-intel_th_alloc(struct device *dev, struct intel_th_drvdata *drvdata,
+intel_th_alloc(struct device *dev, const struct intel_th_drvdata *drvdata,
struct resource *devres, unsigned int ndevres);
void intel_th_free(struct intel_th *th);

@@ -272,7 +272,7 @@ struct intel_th {

struct intel_th_device *thdev[TH_SUBDEVICE_MAX];
struct intel_th_device *hub;
- struct intel_th_drvdata *drvdata;
+ const struct intel_th_drvdata *drvdata;

struct resource resource[TH_MMIO_END];
int (*activate)(struct intel_th *);
diff --git a/drivers/hwtracing/intel_th/pci.c b/drivers/hwtracing/intel_th/pci.c
index 251e75c9ba9d..759994055cb4 100644
--- a/drivers/hwtracing/intel_th/pci.c
+++ b/drivers/hwtracing/intel_th/pci.c
@@ -71,7 +71,7 @@ static void intel_th_pci_deactivate(struct intel_th *th)
static int intel_th_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
- struct intel_th_drvdata *drvdata = (void *)id->driver_data;
+ const struct intel_th_drvdata *drvdata = (void *)id->driver_data;
struct resource resource[TH_MMIO_END + TH_NVEC_MAX] = {
[TH_MMIO_CONFIG] = pdev->resource[TH_PCI_CONFIG_BAR],
[TH_MMIO_SW] = pdev->resource[TH_PCI_STH_SW_BAR],
--
2.30.2

2021-04-15 00:36:25

[permalink] [raw]

Subject: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

From: Andy Shevchenko <[email protected]>

It appears that uuid_t use in STM code abuses UUID API. Moreover,
this type is only useful when we parse user input. Due to above
replace uuid_t with u8 uuid[16] and use uuid_t only when parse
user input.

Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Alexander Shishkin <[email protected]>
---
drivers/hwtracing/stm/p_sys-t.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
index 360b5c03df95..04d13b3785d3 100644
--- a/drivers/hwtracing/stm/p_sys-t.c
+++ b/drivers/hwtracing/stm/p_sys-t.c
@@ -76,7 +76,7 @@ enum sys_t_message_string_subtype {
MIPI_SYST_SEVERITY(MAX))

struct sys_t_policy_node {
- uuid_t uuid;
+ u8 uuid[UUID_SIZE];
bool do_len;
unsigned long ts_interval;
unsigned long clocksync_interval;
@@ -92,7 +92,7 @@ static void sys_t_policy_node_init(void *priv)
{
struct sys_t_policy_node *pn = priv;

- generate_random_uuid(pn->uuid.b);
+ generate_random_uuid(pn->uuid);
}

static int sys_t_output_open(void *priv, struct stm_output *output)
@@ -120,7 +120,7 @@ static ssize_t sys_t_policy_uuid_show(struct config_item *item,
{
struct sys_t_policy_node *pn = to_pdrv_policy_node(item);

- return sprintf(page, "%pU\n", &pn->uuid);
+ return sprintf(page, "%pU\n", pn->uuid);
}

static ssize_t
@@ -129,13 +129,17 @@ sys_t_policy_uuid_store(struct config_item *item, const char *page,
{
struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+ uuid_t uuid;
int ret;

mutex_lock(mutexp);
- ret = uuid_parse(page, &pn->uuid);
+ ret = uuid_parse(page, &uuid);
mutex_unlock(mutexp);
+ if (ret)
+ return ret;

- return ret < 0 ? ret : count;
+ export_uuid(pn->uuid, &uuid);
+ return count;
}

CONFIGFS_ATTR(sys_t_policy_, uuid);
@@ -322,7 +326,7 @@ static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
return sz;

/* GUID */
- sz = stm_data_write(data, m, c, false, op->node.uuid.b, UUID_SIZE);
+ sz = stm_data_write(data, m, c, false, op->node.uuid, sizeof(op->node.uuid));
if (sz <= 0)
return sz;

--
2.30.2

2021-04-15 00:36:26

[permalink] [raw]

Subject: [PATCH 5/7] intel_th: Consistency and off-by-one fix

From: Pavel Machek <[email protected]>

Consistently use "< ... +1" in for loops.

Fix of-by-one in for_each_set_bit().

Signed-off-by: Pavel Machek <[email protected]>
Signed-off-by: Alexander Shishkin <[email protected]>
Link: https://lore.kernel.org/lkml/20190724095841.GA6952@amd/
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/hwtracing/intel_th/gth.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/intel_th/gth.c b/drivers/hwtracing/intel_th/gth.c
index f72803a02391..28509b02a0b5 100644
--- a/drivers/hwtracing/intel_th/gth.c
+++ b/drivers/hwtracing/intel_th/gth.c
@@ -543,7 +543,7 @@ static void intel_th_gth_disable(struct intel_th_device *thdev,
output->active = false;

for_each_set_bit(master, gth->output[output->port].master,
- TH_CONFIGURABLE_MASTERS) {
+ TH_CONFIGURABLE_MASTERS + 1) {
gth_master_set(gth, master, -1);
}
spin_unlock(&gth->gth_lock);
@@ -697,7 +697,7 @@ static void intel_th_gth_unassign(struct intel_th_device *thdev,
othdev->output.port = -1;
othdev->output.active = false;
gth->output[port].output = NULL;
- for (master = 0; master <= TH_CONFIGURABLE_MASTERS; master++)
+ for (master = 0; master < TH_CONFIGURABLE_MASTERS + 1; master++)
if (gth->master[master] == port)
gth->master[master] = -1;
spin_unlock(&gth->gth_lock);
--
2.30.2

2021-04-15 00:36:27

[permalink] [raw]

Subject: [PATCH 6/7] intel_th: pci: Add Rocket Lake CPU support

This adds support for the Trace Hub in Rocket Lake CPUs.

Signed-off-by: Alexander Shishkin <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Cc: stable <[email protected]> # v4.14+
---
drivers/hwtracing/intel_th/pci.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/hwtracing/intel_th/pci.c b/drivers/hwtracing/intel_th/pci.c
index 759994055cb4..a756c995fc7a 100644
--- a/drivers/hwtracing/intel_th/pci.c
+++ b/drivers/hwtracing/intel_th/pci.c
@@ -278,6 +278,11 @@ static const struct pci_device_id intel_th_pci_id_table[] = {
PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x466f),
.driver_data = (kernel_ulong_t)&intel_th_2x,
},
+ {
+ /* Rocket Lake CPU */
+ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x4c19),
+ .driver_data = (kernel_ulong_t)&intel_th_2x,
+ },
{ 0 },
};

--
2.30.2

2021-04-15 00:37:14

[permalink] [raw]

Subject: [PATCH 4/7] intel_th: Constify attribute_group structs

From: Rikard Falkeborn <[email protected]>

The only usage of them is to pass their address to sysfs_create_group()
and sysfs_remove_group(), both which have pointers to const
attribute_group structs as input. Make them const to allow the compiler
to put them in read-only memory.

Signed-off-by: Rikard Falkeborn <[email protected]>
Signed-off-by: Alexander Shishkin <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/hwtracing/intel_th/intel_th.h | 2 +-
drivers/hwtracing/intel_th/msu.c | 2 +-
drivers/hwtracing/intel_th/pti.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/intel_th/intel_th.h b/drivers/hwtracing/intel_th/intel_th.h
index 05fa2dab37d1..89c67e0e1d34 100644
--- a/drivers/hwtracing/intel_th/intel_th.h
+++ b/drivers/hwtracing/intel_th/intel_th.h
@@ -178,7 +178,7 @@ struct intel_th_driver {
/* file_operations for those who want a device node */
const struct file_operations *fops;
/* optional attributes */
- struct attribute_group *attr_group;
+ const struct attribute_group *attr_group;

/* source ops */
int (*set_output)(struct intel_th_device *thdev,
diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 7d95242db900..2edc4666633d 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -2095,7 +2095,7 @@ static struct attribute *msc_output_attrs[] = {
NULL,
};

-static struct attribute_group msc_output_group = {
+static const struct attribute_group msc_output_group = {
.attrs = msc_output_attrs,
};

diff --git a/drivers/hwtracing/intel_th/pti.c b/drivers/hwtracing/intel_th/pti.c
index 0da6b787f553..09132ab8bc23 100644
--- a/drivers/hwtracing/intel_th/pti.c
+++ b/drivers/hwtracing/intel_th/pti.c
@@ -142,7 +142,7 @@ static struct attribute *pti_output_attrs[] = {
NULL,
};

-static struct attribute_group pti_output_group = {
+static const struct attribute_group pti_output_group = {
.attrs = pti_output_attrs,
};

@@ -295,7 +295,7 @@ static struct attribute *lpp_output_attrs[] = {
NULL,
};

-static struct attribute_group lpp_output_group = {
+static const struct attribute_group lpp_output_group = {
.attrs = lpp_output_attrs,
};

--
2.30.2

2021-04-15 00:37:46

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

On Wed, Apr 14, 2021 at 08:12:46PM +0300, Alexander Shishkin wrote:
> From: Andy Shevchenko <[email protected]>
>
> It appears that uuid_t use in STM code abuses UUID API.

How is it being abused?

Moreover,
> this type is only useful when we parse user input. Due to above
> replace uuid_t with u8 uuid[16] and use uuid_t only when parse
> user input.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Alexander Shishkin <[email protected]>
> ---
> drivers/hwtracing/stm/p_sys-t.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
> index 360b5c03df95..04d13b3785d3 100644
> --- a/drivers/hwtracing/stm/p_sys-t.c
> +++ b/drivers/hwtracing/stm/p_sys-t.c
> @@ -76,7 +76,7 @@ enum sys_t_message_string_subtype {
> MIPI_SYST_SEVERITY(MAX))
>
> struct sys_t_policy_node {
> - uuid_t uuid;
> + u8 uuid[UUID_SIZE];

This feels wrong, what is wrong with the uuid_t type usage here?

> bool do_len;
> unsigned long ts_interval;
> unsigned long clocksync_interval;
> @@ -92,7 +92,7 @@ static void sys_t_policy_node_init(void *priv)
> {
> struct sys_t_policy_node *pn = priv;
>
> - generate_random_uuid(pn->uuid.b);

Ok, that's not good, but that looks to be a flaw in the
generate_random_uuid() api, not this driver implementation.

I don't understand why this change is needed?

thanks,

greg k-h

2021-04-15 00:39:11

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

On Wed, Apr 14, 2021 at 07:33:38PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 14, 2021 at 08:12:46PM +0300, Alexander Shishkin wrote:
> > From: Andy Shevchenko <[email protected]>
> >
> > It appears that uuid_t use in STM code abuses UUID API.
>
> How is it being abused?

We are using it against the buffer that is u8, and neither uuid_t nor guid_t.

> Moreover,
> > this type is only useful when we parse user input. Due to above
> > replace uuid_t with u8 uuid[16] and use uuid_t only when parse
> > user input.
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Alexander Shishkin <[email protected]>
> > ---
> > drivers/hwtracing/stm/p_sys-t.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
> > index 360b5c03df95..04d13b3785d3 100644
> > --- a/drivers/hwtracing/stm/p_sys-t.c
> > +++ b/drivers/hwtracing/stm/p_sys-t.c
> > @@ -76,7 +76,7 @@ enum sys_t_message_string_subtype {
> > MIPI_SYST_SEVERITY(MAX))
> >
> > struct sys_t_policy_node {
> > - uuid_t uuid;
> > + u8 uuid[UUID_SIZE];
>
> This feels wrong, what is wrong with the uuid_t type usage here?

Nothing, just will require additional export_uuid() / import_uuid() call.

> > bool do_len;
> > unsigned long ts_interval;
> > unsigned long clocksync_interval;
> > @@ -92,7 +92,7 @@ static void sys_t_policy_node_init(void *priv)
> > {
> > struct sys_t_policy_node *pn = priv;
> >
> > - generate_random_uuid(pn->uuid.b);
>
> Ok, that's not good, but that looks to be a flaw in the
> generate_random_uuid() api, not this driver implementation.
>
> I don't understand why this change is needed?

Using raw buffer APIs against uuid_t / guid_t.

We can import_uuid() first and call uuid_gen() against it. Will it work for
you?

--
With Best Regards,
Andy Shevchenko

2021-04-15 00:42:15

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

On Wed, Apr 14, 2021 at 08:47:48PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 14, 2021 at 07:33:38PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Apr 14, 2021 at 08:12:46PM +0300, Alexander Shishkin wrote:
> > > From: Andy Shevchenko <[email protected]>
> > >
> > > It appears that uuid_t use in STM code abuses UUID API.
> >
> > How is it being abused?
>
> We are using it against the buffer that is u8, and neither uuid_t nor guid_t.

And how should it be used?

> > Moreover,
> > > this type is only useful when we parse user input. Due to above
> > > replace uuid_t with u8 uuid[16] and use uuid_t only when parse
> > > user input.
> > >
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> > > Signed-off-by: Alexander Shishkin <[email protected]>
> > > ---
> > > drivers/hwtracing/stm/p_sys-t.c | 16 ++++++++++------
> > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
> > > index 360b5c03df95..04d13b3785d3 100644
> > > --- a/drivers/hwtracing/stm/p_sys-t.c
> > > +++ b/drivers/hwtracing/stm/p_sys-t.c
> > > @@ -76,7 +76,7 @@ enum sys_t_message_string_subtype {
> > > MIPI_SYST_SEVERITY(MAX))
> > >
> > > struct sys_t_policy_node {
> > > - uuid_t uuid;
> > > + u8 uuid[UUID_SIZE];
> >
> > This feels wrong, what is wrong with the uuid_t type usage here?
>
> Nothing, just will require additional export_uuid() / import_uuid() call.

Isn't that the "correct way" here?

> > > bool do_len;
> > > unsigned long ts_interval;
> > > unsigned long clocksync_interval;
> > > @@ -92,7 +92,7 @@ static void sys_t_policy_node_init(void *priv)
> > > {
> > > struct sys_t_policy_node *pn = priv;
> > >
> > > - generate_random_uuid(pn->uuid.b);
> >
> > Ok, that's not good, but that looks to be a flaw in the
> > generate_random_uuid() api, not this driver implementation.
> >
> > I don't understand why this change is needed?
>
> Using raw buffer APIs against uuid_t / guid_t.

So you want to do that, or you do not want to do that? Totally
confused,

greg k-h

2021-04-15 00:42:23

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

On Wed, Apr 14, 2021 at 08:56:50PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 14, 2021 at 08:47:48PM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 14, 2021 at 07:33:38PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Apr 14, 2021 at 08:12:46PM +0300, Alexander Shishkin wrote:

...

> > Nothing, just will require additional export_uuid() / import_uuid() call.
>
> Isn't that the "correct way" here?

I agree that it is better, yes.

> > > > - generate_random_uuid(pn->uuid.b);
> > >
> > > Ok, that's not good, but that looks to be a flaw in the
> > > generate_random_uuid() api, not this driver implementation.
> > >
> > > I don't understand why this change is needed?
> >
> > Using raw buffer APIs against uuid_t / guid_t.
>
> So you want to do that, or you do not want to do that? Totally
> confused,

It is matter of consistency, so two possibilities here:
- use types _and_ APIs for raw buffer (this patch)
- use types _and_ APIs for uuid_t (suggested in this discussion)

Currently it uses uuid_t type _and_ raw buffer APIs — inconsistency.

--
With Best Regards,
Andy Shevchenko

2021-04-15 00:42:37

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
> Greg Kroah-Hartman <[email protected]> writes:
>
> >> Using raw buffer APIs against uuid_t / guid_t.
> >
> > So you want to do that, or you do not want to do that? Totally
> > confused,
>
> My understanding is that:
> 1) generate_random_uuid() use is allegedly bad even though it's in their
> header,
> 2) poking directly at the byte array inside uuid_t is bad, even though,
> again, header.
>
> It is, indeed, not ideal.
>
> If agreeable, I'll update this patch to the below and respin the whole
> series.

Below patch looks good to me, thanks!

> From 02340f8c7c17ace028040a35553c33cce8f3bce4 Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <[email protected]>
> Date: Wed, 22 Apr 2020 16:02:20 +0300
> Subject: [PATCH] stm class: Use correct UUID APIs
>
> It appears that the STM code didn't manage to accurately decypher the
> delicate inner workings of an alternative thought process behind the
> UUID API and directly called generate_random_uuid() that clearly needs
> to be a static function in lib/uuid.c.
>
> At the same time, said STM code is poking directly at the byte array
> inside the uuid_t when it uses the UUID for its internal purposes.
>
> Fix these two transgressions by using intended APIs instead.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> [ash: changed back to uuid_t and updated the commit message]
> Signed-off-by: Alexander Shishkin <[email protected]>
> ---
> drivers/hwtracing/stm/p_sys-t.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
> index 360b5c03df95..8254971c02e7 100644
> --- a/drivers/hwtracing/stm/p_sys-t.c
> +++ b/drivers/hwtracing/stm/p_sys-t.c
> @@ -92,7 +92,7 @@ static void sys_t_policy_node_init(void *priv)
> {
> struct sys_t_policy_node *pn = priv;
>
> - generate_random_uuid(pn->uuid.b);
> + uuid_gen(&pn->uuid);
> }
>
> static int sys_t_output_open(void *priv, struct stm_output *output)
> @@ -292,6 +292,7 @@ static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
> unsigned int m = output->master;
> const unsigned char nil = 0;
> u32 header = DATA_HEADER;
> + u8 uuid[UUID_SIZE];
> ssize_t sz;
>
> /* We require an existing policy node to proceed */
> @@ -322,7 +323,8 @@ static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
> return sz;
>
> /* GUID */
> - sz = stm_data_write(data, m, c, false, op->node.uuid.b, UUID_SIZE);
> + export_uuid(uuid, &op->node.uuid);
> + sz = stm_data_write(data, m, c, false, uuid, sizeof(op->node.uuid));
> if (sz <= 0)
> return sz;
>
> --
> 2.30.2
>

--
With Best Regards,
Andy Shevchenko

2021-04-15 00:43:31

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

Greg Kroah-Hartman <[email protected]> writes:

>> Using raw buffer APIs against uuid_t / guid_t.
>
> So you want to do that, or you do not want to do that? Totally
> confused,

My understanding is that:
1) generate_random_uuid() use is allegedly bad even though it's in their
header,
2) poking directly at the byte array inside uuid_t is bad, even though,
again, header.

It is, indeed, not ideal.

If agreeable, I'll update this patch to the below and respin the whole
series.

From 02340f8c7c17ace028040a35553c33cce8f3bce4 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <[email protected]>
Date: Wed, 22 Apr 2020 16:02:20 +0300
Subject: [PATCH] stm class: Use correct UUID APIs

It appears that the STM code didn't manage to accurately decypher the
delicate inner workings of an alternative thought process behind the
UUID API and directly called generate_random_uuid() that clearly needs
to be a static function in lib/uuid.c.

At the same time, said STM code is poking directly at the byte array
inside the uuid_t when it uses the UUID for its internal purposes.

Fix these two transgressions by using intended APIs instead.

Signed-off-by: Andy Shevchenko <[email protected]>
[ash: changed back to uuid_t and updated the commit message]
Signed-off-by: Alexander Shishkin <[email protected]>
---
drivers/hwtracing/stm/p_sys-t.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
index 360b5c03df95..8254971c02e7 100644
--- a/drivers/hwtracing/stm/p_sys-t.c
+++ b/drivers/hwtracing/stm/p_sys-t.c
@@ -92,7 +92,7 @@ static void sys_t_policy_node_init(void *priv)
{
struct sys_t_policy_node *pn = priv;

- generate_random_uuid(pn->uuid.b);
+ uuid_gen(&pn->uuid);
}

static int sys_t_output_open(void *priv, struct stm_output *output)
@@ -292,6 +292,7 @@ static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
unsigned int m = output->master;
const unsigned char nil = 0;
u32 header = DATA_HEADER;
+ u8 uuid[UUID_SIZE];
ssize_t sz;

/* We require an existing policy node to proceed */
@@ -322,7 +323,8 @@ static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
return sz;

/* GUID */
- sz = stm_data_write(data, m, c, false, op->node.uuid.b, UUID_SIZE);
+ export_uuid(uuid, &op->node.uuid);
+ sz = stm_data_write(data, m, c, false, uuid, sizeof(op->node.uuid));
if (sz <= 0)
return sz;

--
2.30.2

2021-04-15 08:36:16

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

2021-04-15 08:49:50

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

On Thu, Apr 15, 2021 at 11:35 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
> > Greg Kroah-Hartman <[email protected]> writes:
> >
> > >> Using raw buffer APIs against uuid_t / guid_t.
> > >
> > > So you want to do that, or you do not want to do that? Totally
> > > confused,
> >
> > My understanding is that:
> > 1) generate_random_uuid() use is allegedly bad even though it's in their
> > header,
> > 2) poking directly at the byte array inside uuid_t is bad, even though,
> > again, header.
> >
> > It is, indeed, not ideal.
> >
> > If agreeable, I'll update this patch to the below and respin the whole
> > series.
>
> You are showing that Andy wrote this, when you are the one that did :(

> Anyway, I've dropped this single patch from the series and applied the
> rest. Feel free to send this patch as a stand-alone one once you have
> the authorship issues sorted out.

Internally it was proposed by me as well, so authorship is correct.

--
With Best Regards,
Andy Shevchenko

2021-04-15 09:10:05

[permalink] [raw]

Subject: Re: [PATCH 2/7] stm class: Replace uuid_t with plain u8 uuid[16]

On Thu, Apr 15, 2021 at 11:48:48AM +0300, Andy Shevchenko wrote:
> On Thu, Apr 15, 2021 at 11:35 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
> > > Greg Kroah-Hartman <[email protected]> writes:
> > >
> > > >> Using raw buffer APIs against uuid_t / guid_t.
> > > >
> > > > So you want to do that, or you do not want to do that? Totally
> > > > confused,
> > >
> > > My understanding is that:
> > > 1) generate_random_uuid() use is allegedly bad even though it's in their
> > > header,
> > > 2) poking directly at the byte array inside uuid_t is bad, even though,
> > > again, header.
> > >
> > > It is, indeed, not ideal.
> > >
> > > If agreeable, I'll update this patch to the below and respin the whole
> > > series.
> >
> > You are showing that Andy wrote this, when you are the one that did :(
>
> > Anyway, I've dropped this single patch from the series and applied the
> > rest. Feel free to send this patch as a stand-alone one once you have
> > the authorship issues sorted out.
>
> Internally it was proposed by me as well, so authorship is correct.

And I am supposed to know this how?

Come on people, you know better than this...

2021-04-15 09:21:47