[PATCH 0/7] stm class/intel_th: Updates for v5.13

[PATCH 0/7] stm class/intel_th: Updates for v5.13

[Alexander Shishkin]

Hi Greg,

Here are the stm class and intel_th updates that I have for v5.13. These
are all trivial, including 2 new PCI IDs. Andy provided his reviewed-bys.
Please consider applying. Thank you!

Alexander Shishkin (3):
  intel_th: Constify all drvdata references
  intel_th: pci: Add Rocket Lake CPU support
  intel_th: pci: Add Alder Lake-M support

Andy Shevchenko (1):
  stm class: Replace uuid_t with plain u8 uuid[16]

Jiapeng Chong (1):
  stm class: Remove an unused function

Pavel Machek (1):
  intel_th: Consistency and off-by-one fix

Rikard Falkeborn (1):
  intel_th: Constify attribute_group structs

 drivers/hwtracing/intel_th/core.c     |  2 +-
 drivers/hwtracing/intel_th/gth.c      |  4 ++--
 drivers/hwtracing/intel_th/intel_th.h |  8 ++++----
 drivers/hwtracing/intel_th/msu.c      |  2 +-
 drivers/hwtracing/intel_th/pci.c      | 12 +++++++++++-
 drivers/hwtracing/intel_th/pti.c      |  4 ++--
 drivers/hwtracing/stm/p_sys-t.c       | 16 ++++++++++------
 drivers/hwtracing/stm/policy.c        |  5 -----
 8 files changed, 31 insertions(+), 22 deletions(-)

-- 
2.30.2

[PATCH 1/7] stm class: Remove an unused function

[Alexander Shishkin]

From: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>

Fix the following clang warning:

drivers/hwtracing/stm/policy.c:60:21: warning: unused function
'stp_policy_node_name' [-Wunused-function].

Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hwtracing/stm/policy.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 603b4a9969d3..42103c3a177f 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -57,11 +57,6 @@ void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
 	*cend	= policy_node->last_channel;
 }
 
-static inline char *stp_policy_node_name(struct stp_policy_node *policy_node)
-{
-	return policy_node->group.cg_item.ci_name ? : "<none>";
-}
-
 static inline struct stp_policy *to_stp_policy(struct config_item *item)
 {
 	return item ?
-- 
2.30.2

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

[Alexander Shishkin]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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 <andriy.shevchenko@linux.intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 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

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

[Alexander Shishkin]

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

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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

[PATCH 4/7] intel_th: Constify attribute_group structs

[Alexander Shishkin]

From: Rikard Falkeborn <rikard.falkeborn@gmail.com>

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 <rikard.falkeborn@gmail.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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

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

[Alexander Shishkin]

From: Pavel Machek <pavel@ucw.cz>

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

Fix of-by-one in for_each_set_bit().

Signed-off-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Link: https://lore.kernel.org/lkml/20190724095841.GA6952@amd/
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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

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

[Alexander Shishkin]

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

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: stable <stable@vger.kernel.org> # 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

[PATCH 7/7] intel_th: pci: Add Alder Lake-M support

[Alexander Shishkin]

This adds support for the Trace Hub in Alder Lake-M PCH.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: stable@vger.kernel.org # 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 a756c995fc7a..7da4f298ed01 100644
--- a/drivers/hwtracing/intel_th/pci.c
+++ b/drivers/hwtracing/intel_th/pci.c
@@ -273,6 +273,11 @@ static const struct pci_device_id intel_th_pci_id_table[] = {
 		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x51a6),
 		.driver_data = (kernel_ulong_t)&intel_th_2x,
 	},
+	{
+		/* Alder Lake-M */
+		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x54a6),
+		.driver_data = (kernel_ulong_t)&intel_th_2x,
+	},
 	{
 		/* Alder Lake CPU */
 		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x466f),
-- 
2.30.2

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

[Greg Kroah-Hartman]

On Wed, Apr 14, 2021 at 08:12:46PM +0300, Alexander Shishkin wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> 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 <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  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

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

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
> > 
> > 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 <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > ---
> >  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

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

[Greg Kroah-Hartman]

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 <andriy.shevchenko@linux.intel.com>
> > > 
> > > 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 <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > ---
> > >  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

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

[Andy Shevchenko]

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

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

[Alexander Shishkin]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <andriy.shevchenko@linux.intel.com>
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 <andriy.shevchenko@linux.intel.com>
[ash: changed back to uuid_t and updated the commit message]
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 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

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

[Andy Shevchenko]

On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <andriy.shevchenko@linux.intel.com>
> 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 <andriy.shevchenko@linux.intel.com>
> [ash: changed back to uuid_t and updated the commit message]
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  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

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

[Greg Kroah-Hartman]

On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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.

thanks,

greg k-h

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

[Andy Shevchenko]

On Thu, Apr 15, 2021 at 11:35 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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

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

[Greg Kroah-Hartman]

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
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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...

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

[Alexander Shishkin]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Wed, Apr 14, 2021 at 10:14:34PM +0300, Alexander Shishkin wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 :(

That's intentional, it's Andy's patch. In fact, it was probably me who
insisted on the open-coded-byte-array version, in an offline
conversation some time ago. I'd like to keep his name on it if that's
ok. I've re-sent it [1] as a standalone patch.

> 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.

Thank you!

[1] https://lore.kernel.org/lkml/20210415091555.88085-1-alexander.shishkin@linux.intel.com/

Regards,
--
Alex