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
| ||
|
Date: Tue, 30 Mar 2010 21:23:23 +0530 (IST) From: Pavan Savoy <pavan_savoy@...com> To: Alan Cox <alan@...rguk.ukuu.org.uk> Cc: marcel@...tmann.org, gregkh@...e.de, linux-kernel@...r.kernel.org Subject: Re: [PATCH] drivers:staging: sources for ST core Alan, --- On Tue, 30/3/10, Alan Cox <alan@...rguk.ukuu.org.uk> wrote: > From: Alan Cox <alan@...rguk.ukuu.org.uk> > Subject: Re: [PATCH] drivers:staging: sources for ST core > To: pavan_savoy@...com > Cc: marcel@...tmann.org, gregkh@...e.de, linux-kernel@...r.kernel.org, pavan_savoy@...oo.co.in > Date: Tuesday, 30 March, 2010, 4:52 PM > > +/* all debug macros go in here > */ > > +#define ST_DRV_ERR(fmt, arg...) printk(KERN_ERR > "(stc):"fmt"\n" , ## arg) > > +#if defined(DEBUG) > /* limited debug messages */ > > +#define ST_DRV_DBG(fmt, arg...) > printk(KERN_INFO "(stc):"fmt"\n" , ## arg) > > +#define ST_DRV_VER(fmt, arg...) > > +#elif defined(VERBOSE) > /* very verbose */ > > +#define ST_DRV_DBG(fmt, arg...) > printk(KERN_INFO "(stc):"fmt"\n" , ## arg) > > +#define ST_DRV_VER(fmt, arg...) > printk(KERN_INFO "(stc):"fmt"\n" , ## arg) > > +#else /* error msgs only */ > > +#define ST_DRV_DBG(fmt, arg...) > > +#define ST_DRV_VER(fmt, arg...) > > +#endif > > As Greg said earlier - needs to be using the standard debug > macros Agree - It's all there because of the organization's coding standards. Will correct it. > > > +/* > > + * local data instances > > + */ > > +static struct st_data_s *st_gdata; > > +/* function pointer pointing to either, > > + * st_kim_recv during registration to receive fw > download responses > > + * st_int_recv after registration to receive proto > stack responses > > + */ > > +void (*st_recv) (const unsigned char *data, long > count); > > Need some form of context so you can have multiple device > instances and > avoid games with globals (which always end up in pain) Yes, platform device context ? as in per-device ? However, would face a problem (as mentioned in other mail) - which is when a BT/FM driver which are like client drivers want to use this line discipline, do they need to know which context this needs to go into ? > > + > > > +/********************************************************************/ > > +/* internal misc functions */ > > +bool is_protocol_list_empty(void) > > +{ > > + unsigned char i = 0; > > + ST_DRV_DBG(" %s ", __func__); > > + for (i = 0; i < ST_MAX; i++) { > > + if > (st_gdata->list[i] != NULL) > > + > return ST_NOTEMPTY; > > + /* not empty > */ > > + } > > + /* list empty */ > > + return ST_EMPTY; > > Why not just keep a count of entries you've added/removed > ? One more variable ? to st_gdata ? done. Sounds simple. > > > + * This is the internal write function - a > wrapper > > + * to tty->ops->write > > + */ > > +int st_int_write(const unsigned char *data, int > count) > > +{ > > +#ifdef VERBOSE > /* for debug */ > > + int i; > > +#endif > > + struct tty_struct *tty; > > + if (unlikely(st_gdata == NULL || > st_gdata->tty == NULL)) { > > + ST_DRV_ERR("tty > unavailable to perform write"); > > You need to use the tty pointer passed, you don't want to > walk > tty->something and back because you may race a hangup. > Plus you would > need to manage the krefs (object references) Do I need to do it in tty_open and _release ? Because I don't have anything much to release when tty closes. All I have to do is done in tty_close. > > + return > ST_ERR_FAILURE; > > Should be using Linux error codes for upstream. A global > search/replace > of the ST_ERR_xxx for a similar -Efoo code will do the > trick I think Ok, But is it ok, to maintain codes, internally among ST/BT/FM drivers ? > > +/* > > + * push the skb received to relevant > > + * protocol stacks > > + */ > > +void st_send_frame(enum proto_type protoid, struct > sk_buff *skb) > > +{ > > + ST_DRV_DBG(" %s(prot:%d) ", > __func__, protoid); > > + > > + if (unlikely > > + (st_gdata == NULL || > skb == NULL > > + || > st_gdata->list[protoid] == NULL)) { > > + > ST_DRV_ERR("protocol %d not registered, no data to send?", > > + > protoid); > > + > kfree_skb(skb); > > + return; > > + } > > What is the locking rule to ensure I don't unregister a > protocol as send > frame is called ? send_frame is called from st_int_recv - which is a SOFT-IRQ I believe, So do I really need that ? > > > + * to call registration complete callbacks > > + * of all protocol stack drivers > > + */ > > +void st_reg_complete(char err) > > +{ > > + unsigned char i = 0; > > + ST_DRV_DBG(" %s ", __func__); > > + for (i = 0; i < ST_MAX; i++) { > > + if > (likely(st_gdata != NULL && st_gdata->list[i] != > NULL && > > Except on very hot paths that go odd ways likely and > unlikely are > normally a loss. point taken. > > > +static inline int st_check_data_len(int protoid, int > len) > > +{ > > + register int room = > skb_tailroom(st_gdata->rx_skb); > > No need to use register - gcc is generally smarter than > humans here, > especially in the long term. You may optimise perfectly for > a single > cpu/compiler revision but not for all Ha, this one's a copy/paste piece from hci_ll.c's recv function. > > > +/* Decodes received RAW data and forwards to > corresponding > > + * client drivers (Bluetooth,FM,GPS..etc). > > + * > > + */ > > +void st_int_recv(const unsigned char *data, long > count) > > There are lots of globals here, in part it seems due to the > lack of any > kind of context being passed around > This global function pointer as such is not a necessity. st_recv can point to st_kim_recv (response to firmware download) or st_int_recv (response to commands). They are sort of mutually exclusive, firmware will be downloaded once. and once ready, chip will always use cmd/response st_int_recv. > > > > > + /* this function can be invoked in > more then one context. > > + * so have a lock */ > > It's a good idea to document what data is locked and what > the data > locking assumptions are. I'm finding that part of the code > quite hard to > follow Ok. > > > +/* > > + * internal wakeup function > > + * called from either > > + * - TTY layer when write's finished > > + * - st_write (in context of the protocol stack) > > + */ > > +void st_tx_wakeup(struct st_data_s *st_data) > > > > + while ((skb = > st_int_dequeue(st_data))) { > > Called in two paths but st_int_dequeue seems to have no > internal locking The 2nd path doesn't come until the int_dequeue at all, i.e it would know ST_TX_SENDING, and would queue the skb and return. I remember there was one lock here, removed when I started to have problem on SMP. > > > +/* functions called from ST KIM > > +*/ > > +void kim_st_list_protocols(char *buf) > > +{ > > + unsigned long flags = 0; > > +#ifdef DEBUG > > + unsigned char i = ST_MAX; > > +#endif > > + > spin_lock_irqsave(&st_gdata->lock, flags); > > +#ifdef DEBUG > /* more detailed log */ > > + for (i = 0; i < ST_MAX; i++) { > > + if (i == 0) { > > + > sprintf(buf, "%s is %s", > protocol_strngs[i], > > + > st_gdata->list[i] > != > > + > NULL ? "Registered" : > "Unregistered"); > > Always a good idea to use snprintf and track size (plus > pass buffer size) > it avoida accidents later. Or also see the seq_ interface > when you want > to build proc type files. Ok, the whole thing will go off. The plan is to use /dev/rfkill and avoid the sysfs entry altogether. > > > +/********************************************************************/ > > +/* > > + * functions called from protocol stack drivers > > + * to be EXPORT-ed > > + */ > > +long st_register(struct st_proto_s *new_proto) > > +{ > > + long err = ST_SUCCESS; > > > > > + * functions called from TTY layer > > + */ > > +static int st_tty_open(struct tty_struct *tty) > > +{ > > + int err = ST_SUCCESS; > > + ST_DRV_DBG("%s ", __func__); > > + > > + st_gdata->tty = tty; > > If you do this you need to use krefs and manage the > reference properly > over open/close/hangup ok, But can I really put the kref upon tty_close ? A bluetooth driver, might have registered, but wouldn't have started communication i.e tty_open might not have occurred. So, I wouldn't have any trouble with that right ? > > > +static void st_tty_close(struct tty_struct *tty) > > +{ > > + /* Flush any pending characters in > the driver and discipline. */ > > > + tty_driver_flush_buffer(tty); > > Close will deal with this anyway yep, redundant - will remove it. > > > +static void st_tty_receive(struct tty_struct *tty, > const unsigned char *data, > > + > char *tty_flags, int > count) > > +{ > > +#ifdef VERBOSE > > + long i; > > + printk(KERN_ERR "incoming > data...\n"); > > + for (i = 0; i < count; i++) > > + printk(" %x", > data[i]); > > + printk(KERN_ERR "\n.. data > end\n"); > > +#endif > > + > > + /* > > + * if fw download is in > progress then route incoming data > > + * to KIM for > validation > > + */ > > + st_recv(data, count); > > If you passed tty up and used tty->disc_data you'd be > able to handle > multiple devices Ok, the only problem here is st_int_write, which is called during firmware download. I don't get a tty there, and hence I copy it onto the st_gdata->tty. For the rest of callbacks from TTY layer, I can make use of the tty coming into the function. Considering the other way, if st_gdata went into the tty->disc_data, I have EXPORTED functions like st_register and st_unregister, when I can't get hold of the disc_data. > > > +/********************************************************************/ > > +static int __init st_core_init(void) > > +{ > > + long err; > > + static struct tty_ldisc_ops > *st_ldisc_ops; > > + > > + /* populate and register to TTY > line discipline */ > > + st_ldisc_ops = > kzalloc(sizeof(*st_ldisc_ops), GFP_KERNEL); > > + if (!st_ldisc_ops) { > > + ST_DRV_ERR("no > mem to allocate"); > > + return > -ENOMEM; > > + } > > + > > + st_ldisc_ops->magic = > TTY_LDISC_MAGIC; > > + st_ldisc_ops->name = > "n_st"; /*"n_hci"; */ > > + st_ldisc_ops->open = > st_tty_open; > > + st_ldisc_ops->close = > st_tty_close; > > + st_ldisc_ops->receive_buf = > st_tty_receive; > > + st_ldisc_ops->write_wakeup = > st_tty_wakeup; > > + st_ldisc_ops->flush_buffer = > st_tty_flush_buffer; > > + st_ldisc_ops->owner = > THIS_MODULE; > > You could just declare this as a static struct like other > drivers do ? > Yep, I remember doing that, and using a couple of goto-s as well. Organization coding standards and linux Coding Style collided. - Will do that. The INTERNET now has a personality. YOURS! See your Yahoo! Homepage. http://in.yahoo.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists