* [bug report] coresight: tmc: allocating memory when needed
@ 2018-04-18 12:48 Dan Carpenter
2018-04-18 16:03 ` Mathieu Poirier
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2018-04-18 12:48 UTC (permalink / raw)
To: linux-arm-kernel
[ This bug is a couple years old now so presumably it can't be that
serious but maybe it's still worth looking at? -dan ]
Hello Mathieu Poirier,
The patch de5461970b3e: "coresight: tmc: allocating memory when
needed" from May 3, 2016, leads to the following static checker
warning:
drivers/hwtracing/coresight/coresight-tmc-etr.c:174 tmc_enable_etr_sink_sysfs()
error: uninitialized symbol 'paddr'.
drivers/hwtracing/coresight/coresight-tmc-etr.c
121 static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
122 {
123 int ret = 0;
124 bool used = false;
125 unsigned long flags;
126 void __iomem *vaddr = NULL;
127 dma_addr_t paddr;
^^^^^^^^^^^^^^^^
128 struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
129
130
131 /*
132 * If we don't have a buffer release the lock and allocate memory.
133 * Otherwise keep the lock and move along.
134 */
135 spin_lock_irqsave(&drvdata->spinlock, flags);
136 if (!drvdata->vaddr) {
^^^^^^^^^^^^^^
137 spin_unlock_irqrestore(&drvdata->spinlock, flags);
138
139 /*
140 * Contiguous memory can't be allocated while a spinlock is
141 * held. As such allocate memory here and free it if a buffer
142 * has already been allocated (from a previous session).
143 */
144 vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
145 &paddr, GFP_KERNEL);
146 if (!vaddr)
147 return -ENOMEM;
148
149 /* Let's try again */
150 spin_lock_irqsave(&drvdata->spinlock, flags);
151 }
152
153 if (drvdata->reading) {
154 ret = -EBUSY;
155 goto out;
156 }
157
158 /*
159 * In sysFS mode we can have multiple writers per sink. Since this
160 * sink is already enabled no memory is needed and the HW need not be
161 * touched.
162 */
163 if (drvdata->mode == CS_MODE_SYSFS)
164 goto out;
165
166 /*
167 * If drvdata::buf == NULL, use the memory allocated above.
168 * Otherwise a buffer still exists from a previous session, so
169 * simply use that.
170 */
171 if (drvdata->buf == NULL) {
^^^^^^^^^^^^^^^^^^^^
Perhaps ->buf is only NULL when ->vaddr is NULL? It's surprising that
we're not testing ->buf in both places.
172 used = true;
173 drvdata->vaddr = vaddr;
174 drvdata->paddr = paddr;
^^^^^^^^^^^^^^^^^^^^^^^
175 drvdata->buf = drvdata->vaddr;
176 }
177
178 drvdata->mode = CS_MODE_SYSFS;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* [bug report] coresight: tmc: allocating memory when needed
2018-04-18 12:48 [bug report] coresight: tmc: allocating memory when needed Dan Carpenter
@ 2018-04-18 16:03 ` Mathieu Poirier
0 siblings, 0 replies; 2+ messages in thread
From: Mathieu Poirier @ 2018-04-18 16:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dan,
On 18 April 2018 at 06:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> [ This bug is a couple years old now so presumably it can't be that
> serious but maybe it's still worth looking at? -dan ]
>
> Hello Mathieu Poirier,
>
> The patch de5461970b3e: "coresight: tmc: allocating memory when
> needed" from May 3, 2016, leads to the following static checker
> warning:
>
> drivers/hwtracing/coresight/coresight-tmc-etr.c:174 tmc_enable_etr_sink_sysfs()
> error: uninitialized symbol 'paddr'.
Yes, I can see how a static checker would complain. On the flip side
paddr can't be used uninitialised.
>
> drivers/hwtracing/coresight/coresight-tmc-etr.c
> 121 static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> 122 {
> 123 int ret = 0;
> 124 bool used = false;
> 125 unsigned long flags;
> 126 void __iomem *vaddr = NULL;
> 127 dma_addr_t paddr;
> ^^^^^^^^^^^^^^^^
> 128 struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> 129
> 130
> 131 /*
> 132 * If we don't have a buffer release the lock and allocate memory.
> 133 * Otherwise keep the lock and move along.
> 134 */
> 135 spin_lock_irqsave(&drvdata->spinlock, flags);
> 136 if (!drvdata->vaddr) {
> ^^^^^^^^^^^^^^
> 137 spin_unlock_irqrestore(&drvdata->spinlock, flags);
> 138
> 139 /*
> 140 * Contiguous memory can't be allocated while a spinlock is
> 141 * held. As such allocate memory here and free it if a buffer
> 142 * has already been allocated (from a previous session).
> 143 */
> 144 vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
> 145 &paddr, GFP_KERNEL);
> 146 if (!vaddr)
> 147 return -ENOMEM;
> 148
> 149 /* Let's try again */
> 150 spin_lock_irqsave(&drvdata->spinlock, flags);
> 151 }
> 152
> 153 if (drvdata->reading) {
> 154 ret = -EBUSY;
> 155 goto out;
> 156 }
> 157
> 158 /*
> 159 * In sysFS mode we can have multiple writers per sink. Since this
> 160 * sink is already enabled no memory is needed and the HW need not be
> 161 * touched.
> 162 */
> 163 if (drvdata->mode == CS_MODE_SYSFS)
> 164 goto out;
> 165
> 166 /*
> 167 * If drvdata::buf == NULL, use the memory allocated above.
> 168 * Otherwise a buffer still exists from a previous session, so
> 169 * simply use that.
> 170 */
> 171 if (drvdata->buf == NULL) {
> ^^^^^^^^^^^^^^^^^^^^
>
> Perhaps ->buf is only NULL when ->vaddr is NULL? It's surprising that
> we're not testing ->buf in both places.
You are correct, ->buf is only NULL when ->vaddr is. As such the test
should be the same at both places to avoid confusion. I'll send a
patch where ->vaddr is checked since ->buf is only for compatibility
with the reading function.
Thanks,
Mathieu
>
> 172 used = true;
> 173 drvdata->vaddr = vaddr;
> 174 drvdata->paddr = paddr;
> ^^^^^^^^^^^^^^^^^^^^^^^
>
> 175 drvdata->buf = drvdata->vaddr;
> 176 }
> 177
> 178 drvdata->mode = CS_MODE_SYSFS;
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-04-18 16:03 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-18 12:48 [bug report] coresight: tmc: allocating memory when needed Dan Carpenter
2018-04-18 16:03 ` Mathieu Poirier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.