| Message ID | 20190501092841.9026-1-rasmus.villemoes@prevas.dk |
|---|---|
| Headers | show |
| Series | soc/fsl/qe: cleanups and new DT binding | expand |
Le 01/05/2019 à 11:29, Rasmus Villemoes a écrit : > The current array of struct qe_snum use 256*4 bytes for just keeping > track of the free/used state of each index, and the struct layout > means there's another 768 bytes of padding. If we just unzip that > structure, the array of snum values just use 256 bytes, while the > free/inuse state can be tracked in a 32 byte bitmap. > > So this reduces the .data footprint by 1760 bytes. It also serves as > preparation for introducing another DT binding for specifying the snum > values. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Trivial comment below > --- > drivers/soc/fsl/qe/qe.c | 43 ++++++++++++----------------------------- > 1 file changed, 12 insertions(+), 31 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c > index 855373deb746..303aa29cb27d 100644 > --- a/drivers/soc/fsl/qe/qe.c > +++ b/drivers/soc/fsl/qe/qe.c > @@ -14,6 +14,7 @@ > * Free Software Foundation; either version 2 of the License, or (at your > * option) any later version. > */ > +#include <linux/bitmap.h> > #include <linux/errno.h> > #include <linux/sched.h> > #include <linux/kernel.h> > @@ -43,25 +44,14 @@ static DEFINE_SPINLOCK(qe_lock); > DEFINE_SPINLOCK(cmxgcr_lock); > EXPORT_SYMBOL(cmxgcr_lock); > > -/* QE snum state */ > -enum qe_snum_state { > - QE_SNUM_STATE_USED, > - QE_SNUM_STATE_FREE > -}; > - > -/* QE snum */ > -struct qe_snum { > - u8 num; > - enum qe_snum_state state; > -}; > - > /* We allocate this here because it is used almost exclusively for > * the communication processor devices. > */ > struct qe_immap __iomem *qe_immr; > EXPORT_SYMBOL(qe_immr); > > -static struct qe_snum snums[QE_NUM_OF_SNUM]; /* Dynamically allocated SNUMs */ > +static u8 snums[QE_NUM_OF_SNUM]; /* Dynamically allocated SNUMs */ > +static DECLARE_BITMAP(snum_state, QE_NUM_OF_SNUM); > static unsigned int qe_num_of_snum; > > static phys_addr_t qebase = -1; > @@ -315,10 +305,8 @@ static void qe_snums_init(void) > else > snum_init = snum_init_46; > > - for (i = 0; i < qe_num_of_snum; i++) { > - snums[i].num = snum_init[i]; > - snums[i].state = QE_SNUM_STATE_FREE; > - } > + bitmap_zero(snum_state, QE_NUM_OF_SNUM); > + memcpy(snums, snum_init, qe_num_of_snum); > } > > int qe_get_snum(void) > @@ -328,12 +316,10 @@ int qe_get_snum(void) > int i; > > spin_lock_irqsave(&qe_lock, flags); > - for (i = 0; i < qe_num_of_snum; i++) { > - if (snums[i].state == QE_SNUM_STATE_FREE) { > - snums[i].state = QE_SNUM_STATE_USED; > - snum = snums[i].num; > - break; > - } > + i = find_first_zero_bit(snum_state, qe_num_of_snum); > + if (i < qe_num_of_snum) { > + set_bit(i, snum_state); > + snum = snums[i]; > } > spin_unlock_irqrestore(&qe_lock, flags); > > @@ -343,14 +329,9 @@ EXPORT_SYMBOL(qe_get_snum); > > void qe_put_snum(u8 snum) > { > - int i; > - > - for (i = 0; i < qe_num_of_snum; i++) { > - if (snums[i].num == snum) { > - snums[i].state = QE_SNUM_STATE_FREE; > - break; > - } > - } > + const u8 *p = memchr(snums, snum, qe_num_of_snum); A blank line is expected here. Christophe > + if (p) > + clear_bit(p - snums, snum_state); > } > EXPORT_SYMBOL(qe_put_snum); > >
Le 01/05/2019 à 11:29, Rasmus Villemoes a écrit : > Add driver support for the newly introduced fsl,qe-snums property. > > Conveniently, of_property_read_variable_u8_array does exactly what we > need: If the property fsl,qe-snums is found (and has an allowed size), > the array of values get copied to snums, and the return value is the > number of snums - we cannot assign directly to num_of_snums, since we > need to check whether the return value is negative. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > drivers/soc/fsl/qe/qe.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c > index 0fb8b59f61ad..325d689cbf5c 100644 > --- a/drivers/soc/fsl/qe/qe.c > +++ b/drivers/soc/fsl/qe/qe.c > @@ -283,7 +283,6 @@ EXPORT_SYMBOL(qe_clock_source); > */ > static void qe_snums_init(void) > { > - int i; > static const u8 snum_init_76[] = { > 0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D, > 0x24, 0x25, 0x2C, 0x2D, 0x34, 0x35, 0x88, 0x89, > @@ -304,7 +303,21 @@ static void qe_snums_init(void) > 0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59, > 0x68, 0x69, 0x78, 0x79, 0x80, 0x81, > }; > + struct device_node *qe; > const u8 *snum_init; > + int i; > + > + bitmap_zero(snum_state, QE_NUM_OF_SNUM); > + qe = qe_get_device_node(); > + if (qe) { > + i = of_property_read_variable_u8_array(qe, "fsl,qe-snums", > + snums, 1, QE_NUM_OF_SNUM); > + of_node_put(qe); > + if (i > 0) { > + qe_num_of_snum = i; > + return; > + } > + } > > qe_num_of_snum = qe_get_num_of_snums(); > > @@ -313,7 +326,6 @@ static void qe_snums_init(void) > else > snum_init = snum_init_46; > > - bitmap_zero(snum_state, QE_NUM_OF_SNUM); > memcpy(snums, snum_init, qe_num_of_snum); > } > >
Le 01/05/2019 à 11:29, Rasmus Villemoes a écrit : > The comment "No QE ever has fewer than 28 SNUMs" is false; e.g. the > MPC8309 has 14. The code path returning -EINVAL is also a recipe for > instant disaster, since the caller (qe_snums_init) uncritically > assigns the return value to the unsigned qe_num_of_snum, and would > thus proceed to attempt to copy 4GB from snum_init_46[] to the snum[] > array. > > So fold the handling of the legacy fsl,qe-num-snums into > qe_snums_init, and make sure we do not end up using the snum_init_46 > array in cases other than the two where we know it makes sense. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > drivers/soc/fsl/qe/qe.c | 46 ++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 30 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c > index 325d689cbf5c..276d7d78ebfc 100644 > --- a/drivers/soc/fsl/qe/qe.c > +++ b/drivers/soc/fsl/qe/qe.c > @@ -308,24 +308,33 @@ static void qe_snums_init(void) > int i; > > bitmap_zero(snum_state, QE_NUM_OF_SNUM); > + qe_num_of_snum = 28; /* The default number of snum for threads is 28 */ > qe = qe_get_device_node(); > if (qe) { > i = of_property_read_variable_u8_array(qe, "fsl,qe-snums", > snums, 1, QE_NUM_OF_SNUM); > - of_node_put(qe); > if (i > 0) { > + of_node_put(qe); > qe_num_of_snum = i; > return; > } > + /* > + * Fall back to legacy binding of using the value of > + * fsl,qe-num-snums to choose one of the static arrays > + * above. > + */ > + of_property_read_u32(qe, "fsl,qe-num-snums", &qe_num_of_snum); > + of_node_put(qe); > } > > - qe_num_of_snum = qe_get_num_of_snums(); > - > - if (qe_num_of_snum == 76) > + if (qe_num_of_snum == 76) { > snum_init = snum_init_76; > - else > + } else if (qe_num_of_snum == 28 || qe_num_of_snum == 46) { > snum_init = snum_init_46; > - > + } else { > + pr_err("QE: unsupported value of fsl,qe-num-snums: %u\n", qe_num_of_snum); > + return; > + } > memcpy(snums, snum_init, qe_num_of_snum); > } > > @@ -641,30 +650,7 @@ EXPORT_SYMBOL(qe_get_num_of_risc); > > unsigned int qe_get_num_of_snums(void) > { > - struct device_node *qe; > - int size; > - unsigned int num_of_snums; > - const u32 *prop; > - > - num_of_snums = 28; /* The default number of snum for threads is 28 */ > - qe = qe_get_device_node(); > - if (!qe) > - return num_of_snums; > - > - prop = of_get_property(qe, "fsl,qe-num-snums", &size); > - if (prop && size == sizeof(*prop)) { > - num_of_snums = *prop; > - if ((num_of_snums < 28) || (num_of_snums > QE_NUM_OF_SNUM)) { > - /* No QE ever has fewer than 28 SNUMs */ > - pr_err("QE: number of snum is invalid\n"); > - of_node_put(qe); > - return -EINVAL; > - } > - } > - > - of_node_put(qe); > - > - return num_of_snums; > + return qe_num_of_snum; > } > EXPORT_SYMBOL(qe_get_num_of_snums); > >
On 2019/5/1 17:29, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > -----Original Message----- > From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > Sent: 2019年5月1日 17:29 > To: devicetree@vger.kernel.org; Qiang Zhao <qiang.zhao@nxp.com>; Leo Li > <leoyang.li@nxp.com> > Cc: linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Scott Wood > <oss@buserror.net>; Christophe Leroy <christophe.leroy@c-s.fr>; Mark > Rutland <mark.rutland@arm.com>; Rasmus Villemoes > <Rasmus.Villemoes@prevas.se> > Subject: [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into > qe_snums_init > > The comment "No QE ever has fewer than 28 SNUMs" is false; e.g. the > MPC8309 has 14. The code path returning -EINVAL is also a recipe for instant > disaster, since the caller (qe_snums_init) uncritically assigns the return value to > the unsigned qe_num_of_snum, and would thus proceed to attempt to copy > 4GB from snum_init_46[] to the snum[] array. > > So fold the handling of the legacy fsl,qe-num-snums into qe_snums_init, and > make sure we do not end up using the snum_init_46 array in cases other than > the two where we know it makes sense. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> > drivers/soc/fsl/qe/qe.c | 46 ++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 30 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index > 325d689cbf5c..276d7d78ebfc 100644 > --- a/drivers/soc/fsl/qe/qe.c > +++ b/drivers/soc/fsl/qe/qe.c > @@ -308,24 +308,33 @@ static void qe_snums_init(void) > int i; > > bitmap_zero(snum_state, QE_NUM_OF_SNUM); > + qe_num_of_snum = 28; /* The default number of snum for threads > + is 28 */ > qe = qe_get_device_node(); > if (qe) { > i = of_property_read_variable_u8_array(qe, "fsl,qe-snums", > snums, 1, > QE_NUM_OF_SNUM); > - of_node_put(qe); > if (i > 0) { > + of_node_put(qe); > qe_num_of_snum = i; > return; > } > + /* > + * Fall back to legacy binding of using the value of > + * fsl,qe-num-snums to choose one of the static arrays > + * above. > + */ > + of_property_read_u32(qe, "fsl,qe-num-snums", > &qe_num_of_snum); > + of_node_put(qe); > } > > - qe_num_of_snum = qe_get_num_of_snums(); > - > - if (qe_num_of_snum == 76) > + if (qe_num_of_snum == 76) { > snum_init = snum_init_76; > - else > + } else if (qe_num_of_snum == 28 || qe_num_of_snum == 46) { > snum_init = snum_init_46; > - > + } else { > + pr_err("QE: unsupported value of fsl,qe-num-snums: %u\n", > qe_num_of_snum); > + return; > + } > memcpy(snums, snum_init, qe_num_of_snum); } > > @@ -641,30 +650,7 @@ EXPORT_SYMBOL(qe_get_num_of_risc); > > unsigned int qe_get_num_of_snums(void) > { > - struct device_node *qe; > - int size; > - unsigned int num_of_snums; > - const u32 *prop; > - > - num_of_snums = 28; /* The default number of snum for threads is 28 > */ > - qe = qe_get_device_node(); > - if (!qe) > - return num_of_snums; > - > - prop = of_get_property(qe, "fsl,qe-num-snums", &size); > - if (prop && size == sizeof(*prop)) { > - num_of_snums = *prop; > - if ((num_of_snums < 28) || (num_of_snums > > QE_NUM_OF_SNUM)) { > - /* No QE ever has fewer than 28 SNUMs */ > - pr_err("QE: number of snum is invalid\n"); > - of_node_put(qe); > - return -EINVAL; > - } > - } > - > - of_node_put(qe); > - > - return num_of_snums; > + return qe_num_of_snum; > } > EXPORT_SYMBOL(qe_get_num_of_snums); > > -- > 2.20.1 Best Regards Qiang Zhao