Re: [PATCH] libdvbv5: more fixes in the T2 delivery descriptor handler

[Nikolaus Schulz]

On Wed, Dec 18, 2013 at 10:49:31AM -0200, Mauro Carvalho Chehab wrote:
  * Properly use lengths of centre_frequency loop and subcell_info loop
(they count bytes, not entries)
 
 I'm not sure about this one. On all other similar descriptors, we're storing
 the number of elements at the descriptor's structure, instead of its size,
 as that makes easier to handle them.

OK, I had been looking into other descriptor handlers and didn't spot
anything like that, but isdbt_desc_terrestrial_delivery_system.num_freqs
at least indeed does count array members.

 Also, having all delivery_system descriptors using the number of elements
 makes easier to avoid confusion when writing the handling code.

I agree, it also is way more natural.  Actually I felt that from the
start, but considering that the structs in the code very closely
correlate to the binary DVBSI format, I adopted the byte count
accordingly.

 Btw, I think that this change will likely break the code that handles this
 descriptor at dvb-scan.

Indeed, add_update_nit_dvbt2() would be affected.

 So, could you please redo your patch in order for it to keep storing the
 number of elements at the dvb_desc_t2_delivery::frequency_loop_length
 field? If you prefer, you can rename it to frequency_loop_nmemb (or a
 similar naming) to make it clearer.

I'll re-send it using frequency_loop_nmemb and subcel_info_loop_nmemb,
in order to avoid confusion with the {frequency,subcell_info}_loop_length
fields in the binary DVBSI format.

Thanks,
Nikolaus

  Signed-off-by: Nikolaus Schulz sch...@macnetix.de
  ---
   lib/libdvbv5/descriptors/desc_t2_delivery.c |   35 
  ++-
   1 files changed, 18 insertions(+), 17 deletions(-)
  
  diff --git a/lib/libdvbv5/descriptors/desc_t2_delivery.c 
  b/lib/libdvbv5/descriptors/desc_t2_delivery.c
  index ab4361d..07a0956 100644
  --- a/lib/libdvbv5/descriptors/desc_t2_delivery.c
  +++ b/lib/libdvbv5/descriptors/desc_t2_delivery.c
  @@ -32,6 +32,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms 
  *parms,
  struct dvb_desc_t2_delivery *d = desc;
  unsigned char *p = (unsigned char *) buf;
  size_t desc_len = ext-length - 1, len, len2;
  +   uint8_t nmemb;
  int i;
   
  len = offsetof(struct dvb_desc_t2_delivery, bitfield);
  @@ -42,7 +43,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms 
  *parms,
  return;
  }
  if (desc_len  len2) {
  -   memcpy(p, buf, len);
  +   memcpy(d, p, len);
  bswap16(d-system_id);
   
  if (desc_len != len)
  @@ -50,44 +51,41 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms 
  *parms,
   
  return;
  }
  -   memcpy(p, buf, len2);
  +   memcpy(d, p, len2);
  p += len2;
   
  -   len = desc_len - (p - buf);
  -   memcpy(d-centre_frequency, p, len);
  -   p += len;
  -
  if (d-tfs_flag)
  -   d-frequency_loop_length = 1;
  +   d-frequency_loop_length = sizeof(*d-centre_frequency);
  else {
  d-frequency_loop_length = *p;
  p++;
  }
  +   nmemb = d-frequency_loop_length / sizeof(*d-centre_frequency);
   
  -   d-centre_frequency = calloc(d-frequency_loop_length,
  -sizeof(*d-centre_frequency));
  +   d-centre_frequency = calloc(nmemb, sizeof(*d-centre_frequency));
  if (!d-centre_frequency) {
  dvb_perror(Out of memory);
  return;
  }
   
  -   memcpy(d-centre_frequency, p, sizeof(*d-centre_frequency) * 
  d-frequency_loop_length);
  -   p += sizeof(*d-centre_frequency) * d-frequency_loop_length;
  +   memcpy(d-centre_frequency, p, d-frequency_loop_length);
  +   p += d-frequency_loop_length;
   
  -   for (i = 0; i  d-frequency_loop_length; i++)
  +   for (i = 0; i  nmemb; i++)
  bswap32(d-centre_frequency[i]);
   
  d-subcel_info_loop_length = *p;
  p++;
  +   nmemb = d-subcel_info_loop_length / sizeof(*d-subcell);
   
  -   d-subcell = calloc(d-subcel_info_loop_length, sizeof(*d-subcell));
  +   d-subcell = calloc(nmemb, sizeof(*d-subcell));
  if (!d-subcell) {
  dvb_perror(Out of memory);
  return;
  }
  -   memcpy(d-subcell, p, sizeof(*d-subcell) * d-subcel_info_loop_length);
  +   memcpy(d-subcell, p, d-subcel_info_loop_length);
   
  -   for (i = 0; i  d-subcel_info_loop_length; i++)
  +   for (i = 0; i  nmemb; i++)
  bswap16(d-subcell[i].transposer_frequency);
   }
   
  @@ -97,6 +95,7 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms 
  *parms,
   {
  const struct dvb_desc_t2_delivery *d = desc;
  int i;
  +   uint8_t nmemb;
   
  dvb_log(|   DVB-T2 delivery);
  dvb_log(|   plp_id%d, d-plp_id);
  @@ -113,10 +112,12 @@ void dvb_desc_t2_delivery_print(struct 
  dvb_v5_fe_parms *parms,
  dvb_log(|   bandwidth %d, d-bandwidth);
  dvb_log(|   SISO MISO %d, d-SISO_MISO

Re: [PATCH] libdvbv5: more fixes in the T2 delivery descriptor handler

[Nikolaus Schulz]

Hi Mauro,

I didn't have the time yet to look at your comments and that patch
again, but:

Please note that my email address sch...@macnetix.de is shut down as
of today[1].

Please use my alternate address, n...@htonl.de, for any further replies
or followups.

Maybe Patchwork[2] should also be updated accordingly?

Thanks,
Nikolaus

[1] I don't know if the local admin will disable the old email address
sch...@macnetix.de in time, but I definitely will no longer read
messages sent to it, from now on.
[2] https://patchwork.linuxtv.org/patch/20943/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] libdvbv5: more fixes in the T2 delivery descriptor handler

[Nikolaus Schulz]

* Fix a couple of memcpy calls, and remove a bogus one
* Properly use lengths of centre_frequency loop and subcell_info loop
  (they count bytes, not entries)

Signed-off-by: Nikolaus Schulz sch...@macnetix.de
---
 lib/libdvbv5/descriptors/desc_t2_delivery.c |   35 ++-
 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/lib/libdvbv5/descriptors/desc_t2_delivery.c 
b/lib/libdvbv5/descriptors/desc_t2_delivery.c
index ab4361d..07a0956 100644
--- a/lib/libdvbv5/descriptors/desc_t2_delivery.c
+++ b/lib/libdvbv5/descriptors/desc_t2_delivery.c
@@ -32,6 +32,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
struct dvb_desc_t2_delivery *d = desc;
unsigned char *p = (unsigned char *) buf;
size_t desc_len = ext-length - 1, len, len2;
+   uint8_t nmemb;
int i;
 
len = offsetof(struct dvb_desc_t2_delivery, bitfield);
@@ -42,7 +43,7 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms *parms,
return;
}
if (desc_len  len2) {
-   memcpy(p, buf, len);
+   memcpy(d, p, len);
bswap16(d-system_id);
 
if (desc_len != len)
@@ -50,44 +51,41 @@ void dvb_desc_t2_delivery_init(struct dvb_v5_fe_parms 
*parms,
 
return;
}
-   memcpy(p, buf, len2);
+   memcpy(d, p, len2);
p += len2;
 
-   len = desc_len - (p - buf);
-   memcpy(d-centre_frequency, p, len);
-   p += len;
-
if (d-tfs_flag)
-   d-frequency_loop_length = 1;
+   d-frequency_loop_length = sizeof(*d-centre_frequency);
else {
d-frequency_loop_length = *p;
p++;
}
+   nmemb = d-frequency_loop_length / sizeof(*d-centre_frequency);
 
-   d-centre_frequency = calloc(d-frequency_loop_length,
-sizeof(*d-centre_frequency));
+   d-centre_frequency = calloc(nmemb, sizeof(*d-centre_frequency));
if (!d-centre_frequency) {
dvb_perror(Out of memory);
return;
}
 
-   memcpy(d-centre_frequency, p, sizeof(*d-centre_frequency) * 
d-frequency_loop_length);
-   p += sizeof(*d-centre_frequency) * d-frequency_loop_length;
+   memcpy(d-centre_frequency, p, d-frequency_loop_length);
+   p += d-frequency_loop_length;
 
-   for (i = 0; i  d-frequency_loop_length; i++)
+   for (i = 0; i  nmemb; i++)
bswap32(d-centre_frequency[i]);
 
d-subcel_info_loop_length = *p;
p++;
+   nmemb = d-subcel_info_loop_length / sizeof(*d-subcell);
 
-   d-subcell = calloc(d-subcel_info_loop_length, sizeof(*d-subcell));
+   d-subcell = calloc(nmemb, sizeof(*d-subcell));
if (!d-subcell) {
dvb_perror(Out of memory);
return;
}
-   memcpy(d-subcell, p, sizeof(*d-subcell) * d-subcel_info_loop_length);
+   memcpy(d-subcell, p, d-subcel_info_loop_length);
 
-   for (i = 0; i  d-subcel_info_loop_length; i++)
+   for (i = 0; i  nmemb; i++)
bswap16(d-subcell[i].transposer_frequency);
 }
 
@@ -97,6 +95,7 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms *parms,
 {
const struct dvb_desc_t2_delivery *d = desc;
int i;
+   uint8_t nmemb;
 
dvb_log(|   DVB-T2 delivery);
dvb_log(|   plp_id%d, d-plp_id);
@@ -113,10 +112,12 @@ void dvb_desc_t2_delivery_print(struct dvb_v5_fe_parms 
*parms,
dvb_log(|   bandwidth %d, d-bandwidth);
dvb_log(|   SISO MISO %d, d-SISO_MISO);
 
-   for (i = 0; i  d-frequency_loop_length; i++)
+   nmemb = d-frequency_loop_length / sizeof(*d-centre_frequency);
+   for (i = 0; i  nmemb; i++)
dvb_log(|   centre frequency[%d]   %d, i, 
d-centre_frequency[i]);
 
-   for (i = 0; i  d-subcel_info_loop_length; i++) {
+   nmemb = d-subcel_info_loop_length / sizeof(*d-subcell);
+   for (i = 0; i  nmemb; i++) {
dvb_log(|   cell_id_extension[%d]  %d, i, 
d-subcell[i].cell_id_extension);
dvb_log(|   transposer frequency   %d, 
d-subcell[i].transposer_frequency);
}
-- 
1.7.2.5

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: global mutex in dvb_usercopy (dvbdev.c)

[Nikolaus Schulz]

On Tue, Jan 08, 2013 at 12:05:47PM +0530, Soby Mathew wrote:
 Hi Everyone,
 I have a doubt regarding about the global mutex lock in
 dvb_usercopy(drivers/media/dvb-core/dvbdev.c, line 382) .
 
 
 /* call driver */
 mutex_lock(dvbdev_mutex);
 if ((err = func(file, cmd, parg)) == -ENOIOCTLCMD)
 err = -EINVAL;
 mutex_unlock(dvbdev_mutex);
 
 
 Why is this mutex needed? When I check similar functions like
 video_usercopy, this kind of global locking is not present when func()
 is called.

I cannot say anything about video_usercopy(), but as it happens, there's
a patch[1] queued for Linux 3.9 that will hopefully replace the mutex in
dvb_usercopy() with more fine-grained locking.

Nikolaus

[1] 
http://git.linuxtv.org/media_tree.git/commit/30ad64b8ac539459f8975aa186421ef3db0bb5cb
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] dvb: unlock on error in dvb_ca_en50221_io_do_ioctl()

[Nikolaus Schulz]

On Fri, Jan 04, 2013 at 09:56:02PM +0300, Dan Carpenter wrote:
 We recently pushed the locking down into this function, but there was
 an error path where the unlock was missed.

Ugh, indeed. Thanks for catching this!

Nikolaus.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dvb: push down ioctl lock in dvb_usercopy

[Nikolaus Schulz]

Hi!

I don't want to press or annoy anybody, but may I ask what's the status
of this patch[1]?

On Thu, Jun 21, 2012 at 07:44:15PM +0200, sch...@macnetix.de wrote:
 Since most dvb ioctls wrap their real work with dvb_usercopy, the static mutex
 used in dvb_usercopy effectively is a global lock for dvb ioctls.
 Unfortunately, frontend ioctls can be blocked by the frontend thread for
 several seconds; this leads to unacceptable lock contention.  Mitigate that by
 pushing the mutex from dvb_usercopy down to the individual, device specific
 ioctls.

On our systems, this patch has a great effect in terms of scalability
when operating multiple DVB tuners.

Is it going to be considered?

Thanks,
Nikolaus

[1] http://patchwork.linuxtv.org/patch/12989/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dvb-core: Release semaphore on error path dvb_register_device().

[Nikolaus Schulz]

CC'ing sta...@kernel.org, this bugfix applies to all kernels  2.6.28.

The patch should also be tagged accordingly.

On Sat, Jun 23, 2012 at 05:29:54PM +0530, santosh nayak wrote:
 From: Santosh Nayak santoshprasadna...@gmail.com
 
 There is a missing up_write() here. Semaphore should be released
 before returning error value.
 
 Signed-off-by: Santosh Nayak santoshprasadna...@gmail.com
 ---
 Destination tree linux-next
 
  drivers/media/dvb/dvb-core/dvbdev.c |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/media/dvb/dvb-core/dvbdev.c 
 b/drivers/media/dvb/dvb-core/dvbdev.c
 index 00a6732..39eab73 100644
 --- a/drivers/media/dvb/dvb-core/dvbdev.c
 +++ b/drivers/media/dvb/dvb-core/dvbdev.c
 @@ -243,6 +243,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct 
 dvb_device **pdvbdev,
   if (minor == MAX_DVB_MINORS) {
   kfree(dvbdevfops);
   kfree(dvbdev);
 + up_write(minor_rwsem);
   mutex_unlock(dvbdev_register_lock);
   return -EINVAL;
   }
 -- 
 1.7.4.4
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html