linux-kernel - RE: [PATCH v4 02/12] [media] cxd2880-spi: Add support for CXD2880 SPI interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <02699364973B424C83A42A84B04FDA85440B8B@JPYOKXMS113.jp.sony.com>
Date:   Wed, 20 Dec 2017 00:54:48 +0000
From:   "Takiguchi, Yasunari" <Yasunari.Takiguchi@...y.com>
To:     Mauro Carvalho Chehab <mchehab@...pensource.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "tbird20d@...il.com" <tbird20d@...il.com>,
        "frowand.list@...il.com" <frowand.list@...il.com>,
        "Yamamoto, Masayuki" <Masayuki.Yamamoto@...y.com>,
        "Nozawa, Hideki (STWN)" <Hideki.Nozawa@...y.com>,
        "Yonezawa, Kota" <Kota.Yonezawa@...y.com>,
        "Matsumoto, Toshihiko" <Toshihiko.Matsumoto@...y.com>,
        "Watanabe, Satoshi (SSS)" <Satoshi.C.Watanabe@...y.com>,
        "Takiguchi, Yasunari" <Yasunari.Takiguchi@...y.com>
Subject: RE: [PATCH v4 02/12] [media] cxd2880-spi: Add support for CXD2880
 SPI interface

Hi, Mauro.

> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
> 
> It would be better to use dev_foo() debug macros instead of
> pr_foo() ones.

I got comment for this previous version patch as below
--------------------------------------------------------------------------------------
The best would be to se dev_err() & friends for printing messages, 
as they print the device's name as filled at struct device.
If you don't use, please add a define that will print the name at the logs, like:

  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

either at the begining of the driver or at some header file.

Btw, I'm noticing that you're also using dev_err() on other places of the code. 
Please standardize. OK, on a few places, you may still need to use pr_err(), 
if you need to print a message before initializing struct device, but I suspect that you can init
--------------------------------------------------------------------------------------

You pointed out here before. Because dev_foo () and pr_foo () were mixed.
We standardize with pr_foo() because the logs is outputted before getting the device structure.
Is it better to use dev_foo() where we can use it?


> > +static int cxd2880_stop_feed(struct dvb_demux_feed *feed) {
> > +	int i = 0;
> > +	int ret;
> > +	struct dvb_demux *demux = NULL;
> > +	struct cxd2880_dvb_spi *dvb_spi = NULL;
> > +
> > +	if (!feed) {
> > +		pr_err("invalid arg\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	demux = feed->demux;
> > +	if (!demux) {
> > +		pr_err("feed->demux is NULL\n");
> > +		return -EINVAL;
> > +	}
> > +	dvb_spi = demux->priv;
> > +
> > +	if (!dvb_spi->feed_count) {
> > +		pr_err("no feed is started\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (feed->pid == 0x2000) {
> > +		/*
> > +		 * Special PID case.
> > +		 * Number of 0x2000 feed request was stored
> > +		 * in dvb_spi->all_pid_feed_count.
> > +		 */
> > +		if (dvb_spi->all_pid_feed_count <= 0) {
> > +			pr_err("PID %d not found.\n", feed->pid);
> > +			return -EINVAL;
> > +		}
> > +		dvb_spi->all_pid_feed_count--;
> > +	} else {
> > +		struct cxd2880_pid_filter_config cfgtmp;
> > +
> > +		cfgtmp = dvb_spi->filter_config;
> > +
> > +		for (i = 0; i < CXD2880_MAX_FILTER_SIZE; i++) {
> > +			if (feed->pid == cfgtmp.pid_config[i].pid &&
> > +			    cfgtmp.pid_config[i].is_enable != 0) {
> > +				cfgtmp.pid_config[i].is_enable = 0;
> > +				cfgtmp.pid_config[i].pid = 0;
> > +				pr_debug("removed PID %d from #%d\n",
> > +					 feed->pid, i);
> > +				break;
> > +			}
> > +		}
> > +		dvb_spi->filter_config = cfgtmp;
> > +
> > +		if (i == CXD2880_MAX_FILTER_SIZE) {
> > +			pr_err("PID %d not found\n", feed->pid);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	ret = cxd2880_update_pid_filter(dvb_spi,
> > +					&dvb_spi->filter_config,
> > +					dvb_spi->all_pid_feed_count >
> 0);
> > +	dvb_spi->feed_count--;
> > +
> > +	if (dvb_spi->feed_count == 0) {
> > +		int ret_stop = 0;
> > +
> > +		ret_stop =
> kthread_stop(dvb_spi->cxd2880_ts_read_thread);
> > +		if (ret_stop) {
> > +			pr_err("'kthread_stop failed. (%d)\n",
> ret_stop);
> > +			ret = ret_stop;
> > +		}
> > +		kfree(dvb_spi->ts_buf);
> > +		dvb_spi->ts_buf = NULL;
> > +	}
> > +
> > +	pr_debug("stop feed ok.(count %d)\n", dvb_spi->feed_count);
> > +
> > +	return ret;
> > +}
> 
> I have the feeling that I've seen the code above before at the dvb core.
> Any reason why don't use the already-existing code at dvb_demux.c &
> friends?

The CXD2880 HW PID filter is used to decrease the amount of TS data transferred via limited bit rate SPI interface.

Thanks,
Takiguchi